refactor: migrate live quiz execution (including cache operations) to new architecture #4353
Conversation
…setup for first student UI tests
📝 WalkthroughWalkthroughThis pull request introduces extensive modifications across multiple components and GraphQL operations to transition the application from a session-based model to a live quiz model. Key changes include renaming GraphQL documents and mutation functions, updating the data structure to reflect quiz terminology, and modifying UI elements accordingly. The updates affect components such as Changes
Possibly related PRs
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's limits when posting large numbers of comments.
Actionable comments posted: 16
🧹 Outside diff range and nitpick comments (45)
packages/graphql/src/graphql/ops/MEndLiveQuiz.graphql (1)
1-6: LGTM! Consider adding documentation.The mutation structure is clean and follows GraphQL best practices. Consider adding documentation comments to describe the mutation's purpose and expected behavior.
Add documentation like this:
+# Ends an active live quiz session +# @param id The unique identifier of the live quiz to end +# @returns The quiz id and its updated status mutation EndLiveQuiz($id: String!) { endLiveQuiz(id: $id) { id status } }packages/graphql/src/graphql/ops/MActivateSessionBlock.graphql (1)
Line range hint
1-9: Update the filename to match the new mutation name.The filename
MActivateSessionBlock.graphqlshould be updated toMActivateLiveQuizBlock.graphqlto maintain consistency with the new naming convention. This will:
- Improve code maintainability
- Prevent confusion
- Keep the codebase consistent with the new architecture
packages/graphql/src/graphql/ops/MDeactivateLiveQuizBlock.graphql (1)
1-13: Consider adding documentation comments.To improve maintainability, consider adding GraphQL documentation comments to describe:
- The purpose of the mutation
- The possible status values
- Error scenarios
Example addition:
+""" +Deactivates a specific block in a live quiz. + +@param quizId - The ID of the live quiz +@param blockId - The ID of the block to deactivate +@returns Updated quiz state including: + - status: Current quiz status (e.g., 'ACTIVE', 'COMPLETED') + - activeBlock: Currently active block (null if none) + - blocks: All blocks with their current status +@throws If quiz or block not found + If block already deactivated + If user not authorized +""" mutation DeactivateLiveQuizBlock($quizId: String!, $blockId: Int!) {packages/graphql/src/graphql/ops/QGetRunningLiveQuiz.graphql (2)
25-26: Document the purpose and possible values of new fields.Please clarify the distinction between
typeandelementTypefields. Consider adding comments or documentation to explain:
- The purpose of each field
- The possible values they can contain
- How they differ from each other
Add documentation above the fields:
elements { id + # High-level categorization of the element (e.g., 'QUESTION', 'CONTENT') type + # Specific type of the element (e.g., 'MULTIPLE_CHOICE', 'FREE_TEXT') elementType ...ElementDataWithoutSolutions }
25-26: Consider field order consistency.The new fields are added before the fragment spread, which is good practice. However, consider if
typeandelementTypeshould be grouped withidas they appear to be identity/classification fields.Consider reordering the fields:
elements { id type elementType + # ... other identity/classification fields ...ElementDataWithoutSolutions }apps/frontend-pwa/src/components/hooks/useRemainingInstances.ts (1)
21-29: Improve type safety and storage key management.The code uses 'any' type and lacks storage key namespacing.
+interface StoredResponses { + responses: string[]; +} + +const STORAGE_KEY_PREFIX = 'klicker-uzh:quiz'; + -let storedResponses: any = (await localForage.getItem( - `${quizId}-responses` +let storedResponses: StoredResponses = (await localForage.getItem( + `${STORAGE_KEY_PREFIX}:${quizId}:responses` )) || { responses: [], }packages/shared-components/src/hooks/useSingleStudentResponse.ts (3)
9-19: Add JSDoc documentation for the hook.Consider adding comprehensive JSDoc documentation to improve maintainability and developer experience. Include descriptions of parameters, their types, and the hook's purpose.
+/** + * Hook to manage single student responses based on element instance type. + * @param {Object} params - Hook parameters + * @param {ElementInstance} [params.instance] - The element instance to process + * @param {React.Dispatch<React.SetStateAction<SingleStudentResponseType>>} params.setStudentResponse - State setter for the response + * @param {boolean} [params.defaultRead=false] - Default read state for content elements + */ function useSingleStudentResponse({
25-25: Improve type safety with type guard.The type check using
__typenamecould be more robust with a proper type guard.-if (instance.elementData.__typename === 'ChoicesElementData') { +if (instance.elementData.__typename === 'ChoicesElementData' && 'options' in instance.elementData) {
43-44: Enhance default case documentation.The comment could be more specific about which element types are handled in the default case.
- // default case - valid for FREE_TEXT, NUMERICAL, FLASHCARD elements + // Handle remaining element types (FREE_TEXT, NUMERICAL, FLASHCARD) + // These elements initialize with undefined response and invalid statepackages/graphql/src/graphql/ops/QGetSessionEvaluation.graphql (1)
Consider renaming the query to align with new architecture
The codebase shows a clear transition from "session" to "liveQuiz" terminology, with most new operations using "liveQuiz". The file
QGetSessionEvaluation.graphqlappears to be one of the few remaining files using the old terminology, while mixing it with new "liveQuiz" concepts (as seen withliveQuizLeaderboard). This inconsistency should be addressed by:
- Renaming
QGetSessionEvaluation.graphqltoQGetLiveQuizEvaluation.graphql- Updating the query name from
GetSessionEvaluationtoGetLiveQuizEvaluation- Changing the field
sessionEvaluationtoliveQuizEvaluationThis aligns with other LiveQuiz-related queries in the codebase such as:
GetLiveQuizSummaryGetLiveQuizLeaderboardGetLecturerViewLiveQuizGetControlLiveQuiz🔗 Analysis chain
Line range hint
1-139: Consider renaming the query to align with new architecture.As part of the migration from sessions to live quizzes, the query name
GetSessionEvaluationand the main fieldsessionEvaluationstill use the old terminology while new fields use "liveQuiz". This mixed terminology could be confusing for developers.Consider renaming to maintain consistency:
-query GetSessionEvaluation($id: String!, $hmac: String) { - sessionEvaluation(id: $id, hmac: $hmac) { +query GetLiveQuizEvaluation($id: String!, $hmac: String) { + liveQuizEvaluation(id: $id, hmac: $hmac) {Let's check for any other inconsistencies in terminology across GraphQL operations:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Find GraphQL operations mixing session and live quiz terminology # Expected: No other files should mix these terms echo "Searching for files containing both 'session' and 'liveQuiz' terms..." rg -l "session.*liveQuiz|liveQuiz.*session" --type graphqlLength of output: 253
Script:
#!/bin/bash # Let's try a different approach to search for files containing both terms echo "Searching for files containing both 'session' and 'liveQuiz' terms..." rg -l -g "*.{graphql,gql}" "session.*liveQuiz|liveQuiz.*session" # Let's also check for all GraphQL operation files to understand the naming patterns echo -e "\nListing all GraphQL operation files:" fd -e graphql -e gql # Let's examine the content of GraphQL files containing "session" or "liveQuiz" echo -e "\nExamining content of relevant GraphQL files:" rg -g "*.{graphql,gql}" -A 2 -B 2 "query.*Session|query.*LiveQuiz|type.*Session|type.*LiveQuiz"Length of output: 17924
packages/graphql/src/schema/subscription.ts (1)
34-36: LGTM! Consider documenting the identifier standardization.The changes consistently use
idas the identifier across subscriptions, improving the schema's consistency. The parameter naming is also more specific (e.g.,activityId).Consider adding schema documentation (using GraphQL descriptions) to clarify the identifier usage pattern for future developers.
Example:
singleGroupActivityEnded: t.field({ + description: 'Subscribes to the completion of a specific group activity, identified by its unique id', type: GroupActivityRef, args: { activityId: t.arg.string({ required: true }), },apps/frontend-manage/src/pages/sessions/[id]/cockpit.tsx (3)
3-5: Consider relocating file to align with new architectureWhile the imports have been updated to reflect the live quiz terminology, the file itself remains in the
sessions/[id]directory. This could lead to confusion and maintenance issues.Consider moving this file to a more appropriate location like
live-quizzes/[id]/cockpit.tsxto maintain consistency with the new architecture.
22-26: Align loading state variables with new terminologyWhile the mutation hooks have been updated, the loading state variables still use block-centric terminology. Consider updating them to maintain consistency with the new architecture.
- const [activateLiveQuizBlock, { loading: activatingBlock }] = useMutation( + const [activateLiveQuizBlock, { loading: activatingQuizBlock }] = useMutation( ActivateLiveQuizBlockDocument ) - const [deactivateLiveQuizBlock, { loading: deactivatingBlock }] = useMutation( + const [deactivateLiveQuizBlock, { loading: deactivatingQuizBlock }] = useMutation( DeactivateLiveQuizBlockDocument )Also applies to: 28-29
Line range hint
1-150: Consider a more comprehensive migration strategyWhile the GraphQL operations have been migrated to the new live quiz architecture, several aspects of the application still reference the old session-based model:
- File location:
sessions/[id]/cockpit.tsx- Route navigation:
router.push('/sessions')- Component names and props:
SessionTimeline,sessionId,sessionName- Function names:
handleEndSessionConsider creating a migration plan to:
- Update component names and props systematically
- Implement new routing structure
- Update file organization to reflect the new architecture
- Update all related documentation and comments
This will help maintain consistency and prevent confusion in the codebase.
apps/frontend-pwa/src/components/common/LiveQuizLeaderboard.tsx (4)
23-29: Consider extracting the props type definition.While the inline type definition is concise, extracting it to a named interface would improve maintainability and reusability, especially if this component's props expand in the future.
+interface LiveQuizLeaderboardProps { + quizId: string + className?: string +} + -function LiveQuizLeaderboard({ - quizId, - className, -}: { - quizId: string - className?: string -}): React.ReactElement { +function LiveQuizLeaderboard({ quizId, className }: LiveQuizLeaderboardProps): React.ReactElement {
Line range hint
36-42: Address potential performance bottleneck with network-only fetch policy.The current implementation using
network-onlyfetch policy could overload the server with many simultaneous users. The TODO comments correctly identify that implementing leaderboard updates through subscriptions would be more efficient.Consider prioritizing the implementation of subscription-based updates to prevent potential scalability issues. This would reduce server load and provide real-time updates more efficiently.
Line range hint
44-86: Consider extracting leaderboard delta logic into a custom hook.The current implementation mixes local storage, delta calculation, and UI concerns. Extracting this logic into a custom hook (e.g.,
useLeaderboardDelta) would improve:
- Code organization and reusability
- Testing capabilities
- Separation of concerns
function useLeaderboardDelta(participantId: string, currentEntry: LeaderboardEntry | undefined) { const [blockDelta, setBlockDelta] = useState<BlockResult>(null); useEffect(() => { const calculateDelta = async () => { if (!currentEntry) return; try { const prevStoredEntry: BlockResult = await localforage.getItem( `${participantId}-score-block${currentEntry.lastBlockOrder - 1}` ); if (prevStoredEntry) { setBlockDelta({ score: currentEntry.score - prevStoredEntry.score, rank: currentEntry.rank - prevStoredEntry.rank, }); } } catch (error) { console.error('Failed to calculate leaderboard delta:', error); } }; // Save current entry and calculate delta if (currentEntry) { localforage.setItem( `${participantId}-score-block${currentEntry.lastBlockOrder}`, currentEntry ); if (currentEntry.lastBlockOrder > 0) { calculateDelta(); } } }, [participantId, currentEntry]); return blockDelta; }
93-93: Simplify the leaderboard length check.The current check
leaderboard.length && leaderboard.length > 0is redundant.-{leaderboard.length && leaderboard.length > 0 ? ( +{leaderboard.length > 0 ? (apps/frontend-pwa/src/pages/session/[id].tsx (1)
Line range hint
126-131: Update request body to use quizId instead of sessionId.While the error handling has been simplified, the request body still uses
sessionIdwhich is inconsistent with the new architecture migration.Apply this change to align with the new architecture:
body: JSON.stringify({ instanceId: instanceId, - sessionId: id, + quizId: id, response: { choices: answer }, }),apps/frontend-manage/src/components/sessions/cockpit/SessionTimeline.tsx (2)
121-121: Consider updating remaining session-related terminology.While the button states and labels have been updated to use quiz terminology, there are still several instances of session-related terms that could be updated for consistency:
- Component name could be renamed from
SessionTimelinetoQuizTimeline- Props interface could be renamed from
SessionTimelinePropstoQuizTimelineProps- Data-cy attributes could be updated:
abort-session-cockpit→abort-quiz-cockpitend-session-cockpit→end-quiz-cockpitAlso applies to: 225-225, 270-270, 289-289
Line range hint
241-266: Fix key prop in block mapping.The current implementation uses a fragment (
<>) within the map function without a key prop, which could cause React reconciliation issues. Additionally, the key on theSessionBlockcomponent includes the block status, which might cause unnecessary re-renders if the status changes.Apply this change to fix the key prop issues:
-{blocks.map((block, idx) => ( - <> - <SessionBlock - key={`${block.id}-${block.status}`} - block={block} - inCooldown={inCooldown && activeBlockId === block.id} - setInCooldown={setInCooldown} - active={activeBlockId === block.id} - className="my-auto" - /> - <FontAwesomeIcon - className={twMerge( - 'my-auto', - idx === blocks.length - 1 && 'hidden', - buttonState === 'nextBlock' && - lastActiveBlockId === block.id && - 'text-green-500' - )} - icon={faPauseCircle} - size="xl" - /> - </> -))} +{blocks.map((block, idx) => ( + <React.Fragment key={block.id}> + <SessionBlock + block={block} + inCooldown={inCooldown && activeBlockId === block.id} + setInCooldown={setInCooldown} + active={activeBlockId === block.id} + className="my-auto" + /> + <FontAwesomeIcon + className={twMerge( + 'my-auto', + idx === blocks.length - 1 && 'hidden', + buttonState === 'nextBlock' && + lastActiveBlockId === block.id && + 'text-green-500' + )} + icon={faPauseCircle} + size="xl" + /> + </React.Fragment> +))}packages/graphql/src/public/client.json (1)
Line range hint
1-170: Consider grouping related operations for better maintainability.The live quiz operations are currently scattered throughout the file. Consider grouping related operations together using comments to improve code organization and maintainability. For example:
{ + // Live Quiz Operations "ActivateLiveQuizBlock": "...", "DeactivateLiveQuizBlock": "...", "EndLiveQuiz": "...", "GetLiveQuizLeaderboard": "...", "GetRunningLiveQuiz": "...", + // Session Operations (Legacy) "GetRunningSessions": "...", "GetUnassignedSessions": "...", // ... rest of the operations }packages/graphql/src/schema/query.ts (1)
507-507: Address TODO comment regarding type issues.The TODO comment indicates unresolved type issues after the migration to live quiz. The current workaround using
as anyshould be replaced with proper typing.Would you like me to help resolve these type issues? I can:
- Analyze the type requirements for the live quiz migration
- Propose a properly typed solution
- Create a GitHub issue to track this technical debt
packages/graphql/src/public/schema.graphql (1)
Line range hint
1-1800: Consider documenting GraphQL schema types.The schema would benefit from documentation strings (descriptions) for types, fields, and enums to improve API discoverability and maintainability.
Example documentation for the LiveQuiz type:
+""" +Represents a live quiz instance that can be controlled in real-time. +Contains blocks of questions, feedback mechanisms, and quiz settings. +""" type LiveQuiz { accessMode: LiveQuizAccessMode! activeBlock: ElementBlock # ... rest of the fields }packages/i18n/messages/en.ts (1)
1842-1842: Fix capitalization inconsistencyThe value "End Quiz" has inconsistent capitalization compared to the same key in the cockpit section (line 1345) which uses "End quiz". Consider using consistent capitalization across all occurrences.
Apply this diff to fix the capitalization:
- endQuiz: 'End Quiz', + endQuiz: 'End quiz',packages/graphql/src/ops.schema.json (2)
Line range hint
14270-14321: Consider adding descriptions to mutations.While the mutations are well-structured, adding descriptions would help clarify the differences between ending a live quiz versus a microlearning session, especially regarding any cleanup or state transitions that occur.
{ "name": "endLiveQuiz", - "description": null, + "description": "Ends an active live quiz session, finalizing scores and cleaning up associated resources", "args": [
21491-21527: Consider adding pagination support to the leaderboard query.For better performance and scalability, consider adding pagination parameters (e.g.,
limitandoffsetorcursor-basedpagination) to theliveQuizLeaderboardquery. This would be especially important for quizzes with many participants.Example addition:
{ "name": "liveQuizLeaderboard", "args": [ { "name": "quizId", // ... existing quizId definition }, + { + "name": "first", + "description": "Number of entries to return", + "type": { + "kind": "SCALAR", + "name": "Int", + "ofType": null + } + }, + { + "name": "after", + "description": "Cursor for pagination", + "type": { + "kind": "SCALAR", + "name": "String", + "ofType": null + } + } ],apps/frontend-pwa/src/components/liveSession/QuestionArea.tsx (5)
108-142: SimplifyanswerQuestionfunction logic for improved readabilityThe
answerQuestionfunction contains complex conditional logic, which can be difficult to read and maintain. Refactoring the conditions can enhance clarity and reduce complexity.Consider reorganizing the logic:
const answerQuestion = ({ instanceId, type, input, }: { instanceId: number type: ElementType input: SingleStudentResponseType }): void => { if (!input.valid || typeof input.response === 'undefined') { return } - // Existing complex conditions - // ... + switch (type) { + case ElementType.Sc: + case ElementType.Mc: + case ElementType.Kprim: + if (input.type === type) { + const choicesIdxs = Object.entries(input.response) + .filter(([, value]) => value === true) + .map(([key]) => parseInt(key)) + handleNewResponse(type, instanceId, choicesIdxs) + } + break + case ElementType.FreeText: + if (input.type === ElementType.FreeText) { + handleNewResponse(type, instanceId, input.response) + } + break + case ElementType.Numerical: + if (input.type === ElementType.Numerical) { + handleNewResponse(type, instanceId, String(parseFloat(input.response))) + } + break + default: + // Handle other types if necessary + break + } }
Line range hint
153-178: Avoid unnecessary JSON parsing and stringifying inupdateStoredResponsesWhen working with
localForage, you can store JavaScript objects directly without manually stringifying or parsing. This simplifies the code and reduces potential errors.Apply this diff to use objects directly:
const prevResponses: any = await localForage.getItem( `${quizId}-responses` ) let newResponses: string[] = [] if (Array.isArray(instanceId)) { newResponses = instanceId.map( (id: number) => `${id}-${execution}` ) } else { newResponses = [`${instanceId}-${execution}`] } -const stringified = JSON.stringify( - prevResponses - ? { - responses: [ - ...JSON.parse(prevResponses).responses, - ...newResponses, - ], - timestamp: dayjs().unix(), - } - : { - responses: newResponses, - timestamp: dayjs().unix(), - } -) -await localForage.setItem(`${quizId}-responses`, stringified) +const combinedResponses = prevResponses + ? { + responses: [ + ...prevResponses.responses, + ...newResponses, + ], + timestamp: dayjs().unix(), + } + : { + responses: newResponses, + timestamp: dayjs().unix(), + } +await localForage.setItem(`${quizId}-responses`, combinedResponses)
178-181: Enhance error handling for data consistencyThe
catchblock inupdateStoredResponsessimply logs the error. To ensure data consistency, especially in case of partial saves, consider implementing a rollback mechanism or notifying the user of the failure.Would you like assistance in implementing robust error handling to manage partial saves and ensure data integrity?
211-211: Implement input disabling to prevent double submissionsThe TODO comment indicates the need to disable input during submission to avoid duplicate responses. Implementing this feature enhances user experience and prevents potential data inconsistencies.
Apply this diff to disable inputs while submitting:
+const [submitting, setSubmitting] = useState(false) const onSubmit = async (): Promise<void> => { + setSubmitting(true) // existing submission logic // ... + setSubmitting(false) } const onExpire = async (): Promise<void> => { + setSubmitting(true) // existing expiration logic // ... + setSubmitting(false) } // In the JSX render <StudentElement element={currentInstance} elementIx={activeInstance} singleStudentResponse={studentResponse} setSingleStudentResponse={setStudentResponse} hideReadButton - // disabledInput={submitting} // TODO: add to avoid double submission + disabledInput={submitting} />Ensure that the
submittingstate accurately reflects the submission status to prevent unintended behavior.
211-211: Remove unnecessary prophideReadButtonif it's no longer usedThe
hideReadButtonprop is passed toStudentElement. If this prop is deprecated or has no effect, consider removing it to clean up the code.<StudentElement element={currentInstance} elementIx={activeInstance} singleStudentResponse={studentResponse} setSingleStudentResponse={setStudentResponse} - hideReadButton disabledInput={submitting} />packages/shared-components/src/StudentElement.tsx (3)
96-121: RefactorsetResponsehandlers to eliminate code duplicationThe
setResponsefunction within theFlashcardcomponent contains duplicated code for updating the student responses based on whetherstudentResponseorsingleStudentResponseis used. Refactoring this logic into a shared helper function can enhance maintainability and readability.Implement a helper function to update responses:
// Define this inside the StudentElement component const updateResponse = (newData: Partial<SingleStudentResponseType>) => { if (mode === 'stack') { setStudentResponse((response) => ({ ...response, [element.id]: { ...response[element.id], ...newData, }, })) } else { setSingleStudentResponse((response) => ({ ...response, ...newData, })) } }Modify the
setResponsehandler:<Flashcard // ...other props setResponse={(studentResponse) => { - if (mode === 'stack') { - setStudentResponse((response) => ({ - ...response, - [element.id]: { - ...response[element.id], - type: ElementType.Flashcard, - response: studentResponse, - valid: true, - }, - })) - } else { - setSingleStudentResponse((response) => ({ - ...response, - type: ElementType.Flashcard, - response: studentResponse, - valid: true, - })) - } + updateResponse({ + type: ElementType.Flashcard, + response: studentResponse, + valid: true, + }) }} />This approach can be similarly applied to other components like
ChoicesQuestion,NumericalQuestion, andFreeTextQuestion.
137-161: Apply response update helper to other question componentsThe
setResponsehandlers inChoicesQuestion,NumericalQuestion, andFreeTextQuestioncomponents also have duplicated logic for updating responses. Refactor these handlers to use theupdateResponsehelper function introduced previously to enhance code consistency and reduce duplication.For example, in the
ChoicesQuestioncomponent:<ChoicesQuestion // ...other props setResponse={(newValue, valid) => { - if (mode === 'stack') { - setStudentResponse((response) => ({ - ...response, - [element.id]: { - ...response[element.id], - type: element.elementData.type as ElementChoicesType, - response: newValue, - valid: valid, - }, - })) - } else { - setSingleStudentResponse((response) => ({ - ...response, - type: element.elementData.type as ElementChoicesType, - response: newValue, - valid: valid, - })) - } + updateResponse({ + type: element.elementData.type as ElementChoicesType, + response: newValue, + valid: valid, + }) }} />Apply similar changes to
NumericalQuestionandFreeTextQuestion.Also applies to: 181-211, 229-259
83-87: Destructure props consistently for clarityIn the
StudentElementfunction, consider destructuring all necessary props at the beginning for consistency and clarity. This makes it easier to see which props are being used and aligns with common React practices.Modify the destructuring as follows:
function StudentElement(props: StudentElementStackProps | StudentElementSingleProps) { const { element, elementIx, hideReadButton = false, disabledInput = false, mode, + studentResponse, + setStudentResponse, + singleStudentResponse, + setSingleStudentResponse, + stackStorage, } = props const evaluation = mode === 'stack' ? stackStorage?.[element.id]?.evaluation : undefined // ...rest of the component }packages/graphql/src/services/sessions.ts (6)
Line range hint
24-27: Avoid usinganytype; define specific types for better type safetyUsing
anyin TypeScript reduces type safety and can lead to runtime errors. Please define proper interfaces or types forcachedResultsandactiveBlockinProcessCachedDataArgsto enhance maintainability and catch errors at compile time.
Line range hint
58-93: Ensure proper type definitions forinstanceResultsto improve readabilityThe
instanceResultsvariable is being constructed with complex nested structures without explicit type annotations. Defining a specific interface forinstanceResultswould improve code clarity and maintainability.
Line range hint
172-174: Correct the type assertion ofsd;stdreturns a number, notnumber[]In the
computeStatisticsfunction, thestdfunction is being type asserted asnumber[]:sd: std(valueArray) as number[],However,
std(valueArray)returns anumber, not an array. The type assertion should beas number.Apply this diff to fix the type assertion:
-sd: std(valueArray) as number[], +sd: std(valueArray) as number,
Line range hint
144-156: Trim input values before comparison in free-text answer validationWhen comparing free-text answers with solutions, consider trimming whitespace from both the student's answer and the solutions to avoid mismatches due to leading or trailing spaces.
Apply these changes in the
checkCorrectnessFreeTextfunction:const solutions = instance.questionData.options.solutions.map( - (solution: string) => solution.toLowerCase() + (solution: string) => solution.trim().toLowerCase() ) ... if (solutions.includes(instance.results[id].value.toLowerCase())) { + if (solutions.includes(instance.results[id].value.trim().toLowerCase())) { instance.results[id].correct = true } else { instance.results[id].correct = false }
Line range hint
237-240: Handle potential undefinedprocess.env.APP_SECRETIf
APP_SECRETis undefined,createHmacwill throw an error. Ensure thatprocess.env.APP_SECRETis defined or provide a fallback mechanism.Consider adding a check before using
process.env.APP_SECRET:if (!process.env.APP_SECRET) { throw new Error('APP_SECRET environment variable is not defined') }
Line range hint
1-400: Consistent naming: updatesessionterminology toliveQuizThroughout the code, consider updating variable and function names from
sessiontoliveQuizto reflect the new architecture and improve code clarity.packages/graphql/src/schema/mutation.ts (2)
691-698: Ensure documentation reflectsendLiveQuizchangesThe mutation
endLiveQuizhas been updated fromendSession, and the return type changed fromSessiontoLiveQuiz. Please ensure that all relevant documentation and API references are updated to reflect these changes.
776-784: Consistent argument naming indeactivateLiveQuizBlockThe arguments have been renamed from
sessionIdandsessionBlockIdtoquizIdandblockId. Verify that this naming convention is consistently applied across the codebase and that any client applications consuming this API are updated accordingly.packages/graphql/src/services/liveQuizzes.ts (2)
716-716: Simplify Conditional Using Optional ChainingYou can simplify the conditional check on line 716 by using optional chaining, making the code cleaner and more concise.
Apply this diff:
- if (liveQuiz.activeBlock && liveQuiz.activeBlock.id) { + if (liveQuiz.activeBlock?.id) {🧰 Tools
🪛 Biome
[error] 716-716: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
1103-1426: Consider RefactoringendLiveQuizFunction for MaintainabilityThe
endLiveQuizfunction spans over 300 lines and handles multiple responsibilities, which can affect readability and maintainability. Consider refactoring it into smaller helper functions to improve modularity and reduce complexity.
🛑 Comments failed to post (16)
packages/graphql/src/graphql/ops/MActivateSessionBlock.graphql (1)
1-2: 💡 Codebase verification
Rename file to match the new mutation name
The verification shows:
- No references to the old mutation name
ActivateSessionBlockin TypeScript files- Only one legacy reference to
sessionIdandsessionBlockIdinsessions.ts, which appears to be a Redis-related implementation- Looking at other GraphQL files, there's a clear naming pattern where mutation files are prefixed with 'M' followed by the mutation name (e.g.,
MActivateLiveQuizBlock,MDeactivateLiveQuizBlock)The file should be renamed from
MActivateSessionBlock.graphqltoMActivateLiveQuizBlock.graphqlto maintain consistency with:
- The new mutation name
- The established naming pattern in the codebase
- The live quiz architecture migration
🔗 Analysis chain
LGTM! Verify dependent components.
The mutation changes align well with the live quiz migration. The parameter renaming from
sessionId/sessionBlockIdtoquizId/blockIdis consistent with the new architecture.Let's verify that all components have been updated to use the new mutation name and parameters:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any remaining references to the old mutation name rg "ActivateSessionBlock|activateSessionBlock" --type ts --type tsx # Search for any remaining references to the old parameter names rg "sessionId.*sessionBlockId" --type ts --type tsxLength of output: 179
Script:
#!/bin/bash # Search for any remaining references to the old mutation name in all files rg "ActivateSessionBlock|activateSessionBlock" # Search for any remaining references to the old parameter names in all files rg "sessionId.*sessionBlockId" # List all GraphQL operation files to check naming patterns fd ".graphql$" packages/graphql/src/graphql/ops/Length of output: 11962
apps/frontend-pwa/src/components/hooks/useRemainingInstances.ts (3)
45-47: 🛠️ Refactor suggestion
Improve error handling.
Currently, errors are only logged to console without user feedback.
Consider implementing proper error handling:
- Add error state management
- Provide user feedback
- Implement retry mechanism
+interface Props { + // ... existing props ... + onError?: (error: Error) => void; +} } catch (e) { - console.error(e) + console.error('Failed to fetch remaining instances:', e); + onError?.(e instanceof Error ? e : new Error('Unknown error occurred')); }Committable suggestion skipped: line range outside the PR's diff.
31-41:
⚠️ Potential issueFix O(n²) complexity in reducer.
The spread operator in the reducer creates O(n²) complexity. Let's optimize it.
-const remaining = instances - .map((instance) => instance.id) - .reduce<number[]>((indices, instanceId, index) => { - if ( - storedResponses?.responses?.includes(`${instanceId}-${execution}`) - ) { - return indices - } - return [...indices, index] - }, []) +const remaining: number[] = []; +instances.forEach((instance, index) => { + if (!storedResponses?.responses?.includes(`${instance.id}-${execution}`)) { + remaining.push(index); + } +});📝 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.const remaining: number[] = []; instances.forEach((instance, index) => { if (!storedResponses?.responses?.includes(`${instance.id}-${execution}`)) { remaining.push(index); } });🧰 Tools
🪛 Biome
[error] 40-40: Avoid the use of spread (
...) syntax on accumulators.Spread syntax should be avoided on accumulators (like those in
.reduce) because it causes a time complexity ofO(n^2).
Consider methods such as .splice or .push instead.(lint/performance/noAccumulatingSpread)
18-50: 🛠️ Refactor suggestion
Add cleanup function to prevent memory leaks.
The useEffect hook should include a cleanup function to handle component unmounting.
useEffect((): void => { + let mounted = true; const exec = async () => { try { let storedResponses: any = (await localForage.getItem( `${quizId}-responses` )) || { responses: [], } // ... rest of the code ... - setActiveInstance(remaining[0]) - setRemainingQuestions(remaining) + if (mounted) { + setActiveInstance(remaining[0]) + setRemainingQuestions(remaining) + } } catch (e) { console.error(e) } } exec() + return () => { + mounted = false; + } }, [quizId, instances, execution])📝 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.useEffect((): void => { let mounted = true; const exec = async () => { try { let storedResponses: any = (await localForage.getItem( `${quizId}-responses` )) || { responses: [], } if (typeof storedResponses === 'string') { storedResponses = JSON.parse(storedResponses) } const remaining = instances .map((instance) => instance.id) .reduce<number[]>((indices, instanceId, index) => { if ( storedResponses?.responses?.includes(`${instanceId}-${execution}`) ) { return indices } return [...indices, index] }, []) if (mounted) { setActiveInstance(remaining[0]) setRemainingQuestions(remaining) } } catch (e) { console.error(e) } } exec() return () => { mounted = false; } }, [quizId, instances, execution])🧰 Tools
🪛 Biome
[error] 40-40: Avoid the use of spread (
...) syntax on accumulators.Spread syntax should be avoided on accumulators (like those in
.reduce) because it causes a time complexity ofO(n^2).
Consider methods such as .splice or .push instead.(lint/performance/noAccumulatingSpread)
packages/shared-components/src/hooks/useSingleStudentResponse.ts (1)
28-33: 🛠️ Refactor suggestion
Optimize object initialization performance.
The current implementation using spread operator in reduce has O(n²) complexity. Consider using Object.fromEntries for better performance.
- response: instance.elementData.options.choices.reduce( - (acc, _, ix) => { - return { ...acc, [ix]: undefined } - }, - {} as Record<number, boolean | undefined> - ), + response: Object.fromEntries( + instance.elementData.options.choices.map((_, ix) => [ix, undefined]) + ) as Record<number, boolean | undefined>,📝 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.response: Object.fromEntries( instance.elementData.options.choices.map((_, ix) => [ix, undefined]) ) as Record<number, boolean | undefined>,🧰 Tools
🪛 Biome
[error] 30-30: Avoid the use of spread (
...) syntax on accumulators.Spread syntax should be avoided on accumulators (like those in
.reduce) because it causes a time complexity ofO(n^2).
Consider methods such as .splice or .push instead.(lint/performance/noAccumulatingSpread)
apps/frontend-manage/src/pages/sessions/[id]/cockpit.tsx (1)
97-109: 💡 Codebase verification
🛠️ Refactor suggestion
Complete the terminology migration in SessionTimeline component
The verification confirms that the old session terminology is still present in multiple places:
- The component file is still named
SessionTimeline.tsxinapps/frontend-manage/src/components/sessions/cockpit/- The interface uses
sessionNameandhandleEndSessionprops- The component uses
sessionIdandsessionNameparameters- The component contains
isFeedbackSessionvariable🔗 Analysis chain
Complete the terminology migration
While the mutation calls have been updated, there are still references to the old session-based terminology:
- The
handleEndSessionfunction name- The
sessionIdprop in theSessionTimelinecomponent- The
SessionTimelinecomponent name itselfConsider updating these for consistency:
- handleEndSession={() => { + handleEndLiveQuiz={() => { endLiveQuiz({ variables: { id: id } }) router.push('/sessions') }}Let's verify if the SessionTimeline component has been migrated in its definition:
Let me try a different search strategy to locate the SessionTimeline component and verify the terminology usage.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for the SessionTimeline component definition ast-grep --pattern 'const SessionTimeline = $_'Length of output: 587
Script:
#!/bin/bash # Search for files containing SessionTimeline rg "SessionTimeline" -l # Search for the actual component implementation with context rg "SessionTimeline" -A 5Length of output: 3259
packages/graphql/src/public/server.json (1)
149-149: 🛠️ Refactor suggestion
Consider migrating the session evaluation query to live quiz context.
The
GetSessionEvaluationquery appears to be using session-based terminology. As part of the architecture migration, consider:
- Renaming to
GetLiveQuizEvaluationfor consistency- Updating parameters from session-related to quiz-related
- Reviewing the return type structure for any needed updates
Let's verify the usage of this query:
#!/bin/bash # Description: Find files that use the GetSessionEvaluation query # to assess the impact of renaming it. rg -l "GetSessionEvaluation|sessionEvaluation"apps/frontend-pwa/src/components/liveSession/QuestionArea.tsx (2)
85-85:
⚠️ Potential issueEnsure
activeInstanceis updated correctly when no questions remainWhen all questions have been answered,
newRemaining[0]may beundefined, leading toactiveInstancebeing set to0unintentionally. This could cause the first question to reappear.Apply this diff to handle the scenario properly:
setActiveInstance(newRemaining[0] || 0) +if (newRemaining.length === 0) { + // No remaining questions; handle accordingly. + // For example, navigate away or display a completion message. +}Committable suggestion skipped: line range outside the PR's diff.
41-43:
⚠️ Potential issueHandle potential empty
instancesarray to prevent runtime errorsIf the
instancesarray is empty, accessinginstances[activeInstance]will result inundefined, leading to runtime errors whencurrentInstanceis used. Consider adding a check to ensure thatinstancesis not empty before proceeding.Apply this diff to handle the empty
instancesarray:const [activeInstance, setActiveInstance] = useState<number>(0) -const currentInstance = instances[activeInstance] +const currentInstance = instances[activeInstance] || null +if (!currentInstance) { + // Handle the case where there is no current instance + return <div>{t('pwa.session.noQuestionsAvailable')}</div> +}📝 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.const [activeInstance, setActiveInstance] = useState<number>(0) const currentInstance = instances[activeInstance] || null if (!currentInstance) { // Handle the case where there is no current instance return <div>{t('pwa.session.noQuestionsAvailable')}</div> }packages/shared-components/src/StudentElement.tsx (1)
61-74: 🛠️ Refactor suggestion
Enhance type safety using discriminated unions in props interfaces
Currently, the component uses type assertions and checks like
typeof studentResponse !== 'undefined'to determine which props are available, which can be error-prone and makes the code harder to maintain. Consider using discriminated unions by adding a literalmodeproperty to your props interfaces. This allows TypeScript to narrow down the type based on themodeproperty, improving type safety and code readability.Apply the following changes:
Update
StudentElementStackPropsandStudentElementSinglePropsto include amodeproperty:interface StudentElementStackProps extends StudentElementBaseProps { + mode: 'stack' studentResponse: StudentResponseType setStudentResponse: Dispatch<SetStateAction<StudentResponseType>> stackStorage?: StudentResponseType singleStudentResponse?: never setSingleStudentResponse?: never } interface StudentElementSingleProps extends StudentElementBaseProps { + mode: 'single' studentResponse?: never setStudentResponse?: never stackStorage?: never singleStudentResponse: SingleStudentResponseType setSingleStudentResponse: Dispatch<SetStateAction<SingleStudentResponseType>> }Modify the
StudentElementcomponent to use themodefor type narrowing:function StudentElement(props: StudentElementStackProps | StudentElementSingleProps) { + const { mode } = props const { element, elementIx, hideReadButton = false, disabledInput = false } = props + const evaluation = + mode === 'stack' ? props.stackStorage?.[element.id]?.evaluation : undefined if (element.elementData.__typename === 'FlashcardElementData') { return ( <Flashcard key={element.id} content={element.elementData.content} explanation={element.elementData.explanation!} - response={ - typeof studentResponse !== 'undefined' - ? (studentResponse[element.id]?.response as FlashcardCorrectness) - : (singleStudentResponse.response as FlashcardCorrectness) - } + response={ + mode === 'stack' + ? (props.studentResponse[element.id]?.response as FlashcardCorrectness) + : (props.singleStudentResponse.response as FlashcardCorrectness) + } setResponse={(studentResponse) => { - typeof setStudentResponse !== 'undefined' + if (mode === 'stack') { props.setStudentResponse((response) => ({ ...response, [element.id]: { ...response[element.id], type: ElementType.Flashcard, response: studentResponse, valid: true, }, })) - }) - : setSingleStudentResponse((response) => { + } else { props.setSingleStudentResponse((response) => ({ ...response, type: ElementType.Flashcard, response: studentResponse, valid: true, })) }) + } }} existingResponse={ mode === 'stack' ? (props.stackStorage?.[element.id]?.response as FlashcardCorrectness) : undefined } elementIx={elementIx} /> ) } // ... handle other element types similarly }This change improves type safety by leveraging TypeScript's discriminated unions and reduces the need for repetitive
typeofchecks.Committable suggestion skipped: line range outside the PR's diff.
packages/graphql/src/services/liveQuizzes.ts (6)
37-38:
⚠️ Potential issueRework Scheduling for Serverless Environment
In lines 37-38, the use of global variables like
scheduledJobsmay lead to unexpected behavior in a serverless environment due to stateless execution contexts. Consider reworking the scheduling mechanism to be compatible with serverless architectures.
176-181:
⚠️ Potential issuePerformance Issue: Avoid Spread Syntax in Reducer
In lines 176-181, modifying the accumulator with spread syntax is inefficient. Directly mutate the accumulator.
Apply this diff:
- return { - ...acc, - [instance.id]: { - ...acc[instance.id], - responseHashes: cacheObj, - }, - } + acc[instance.id] = { + ...acc[instance.id], + responseHashes: cacheObj, + }; + return acc;📝 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.acc[instance.id] = { ...acc[instance.id], responseHashes: cacheObj, }; return acc;🧰 Tools
🪛 Biome
[error] 176-176: Avoid the use of spread (
...) syntax on accumulators.Spread syntax should be avoided on accumulators (like those in
.reduce) because it causes a time complexity ofO(n^2).
Consider methods such as .splice or .push instead.(lint/performance/noAccumulatingSpread)
155-160:
⚠️ Potential issuePerformance Issue: Avoid Spread Syntax in Reducer
In lines 155-160, using spread syntax to create a new object on each iteration affects performance. Mutate the accumulator directly to prevent unnecessary object creation.
Apply this diff:
- return { - ...acc, - [instance.id]: { - ...acc[instance.id], - anonymousResults, - }, - } + acc[instance.id] = { + ...acc[instance.id], + anonymousResults, + }; + return acc;📝 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.acc[instance.id] = { ...acc[instance.id], anonymousResults, }; return acc;🧰 Tools
🪛 Biome
[error] 155-155: Avoid the use of spread (
...) syntax on accumulators.Spread syntax should be avoided on accumulators (like those in
.reduce) because it causes a time complexity ofO(n^2).
Consider methods such as .splice or .push instead.(lint/performance/noAccumulatingSpread)
166-171:
⚠️ Potential issuePerformance Issue: Avoid Spread Syntax in Reducer
Again, in lines 166-171, avoid using spread syntax in reducers to prevent performance degradation.
Apply this diff:
- return { - ...acc, - [instance.id]: { - ...acc[instance.id], - responses: cacheObj, - }, - } + acc[instance.id] = { + ...acc[instance.id], + responses: cacheObj, + }; + return acc;📝 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.acc[instance.id] = { ...acc[instance.id], responses: cacheObj, }; return acc;🧰 Tools
🪛 Biome
[error] 166-166: Avoid the use of spread (
...) syntax on accumulators.Spread syntax should be avoided on accumulators (like those in
.reduce) because it causes a time complexity ofO(n^2).
Consider methods such as .splice or .push instead.(lint/performance/noAccumulatingSpread)
117-119:
⚠️ Potential issuePerformance Issue: Avoid Spread Syntax in Reducer
Using the spread syntax
...within a reducer can lead to O(n²) time complexity because it creates a new object on each iteration. In lines 117-119, consider mutating the accumulator directly to improve performance.Apply this diff to fix the performance issue:
- return { - ...acc, - [responseHash]: (acc[responseHash] ?? 0) + parseInt(count), - } + acc[responseHash] = (acc[responseHash] ?? 0) + parseInt(count); + return acc;📝 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.acc[responseHash] = (acc[responseHash] ?? 0) + parseInt(count); return acc;🧰 Tools
🪛 Biome
[error] 117-117: Avoid the use of spread (
...) syntax on accumulators.Spread syntax should be avoided on accumulators (like those in
.reduce) because it causes a time complexity ofO(n^2).
Consider methods such as .splice or .push instead.(lint/performance/noAccumulatingSpread)
135-143:
⚠️ Potential issuePerformance Issue: Avoid Spread Syntax in Reducer
Similarly, in lines 135-143, using spread syntax in the reducer causes performance degradation. Mutate the accumulator directly to enhance performance.
Apply this diff:
- return { - ...responses_acc, - [responseHash]: { - value: - acc[instance.id]?.['responseHashes'][responseHash] ?? responseHash, - count: - (responses_acc[responseHash]?.count ?? 0) + parseInt(count), - }, - } + responses_acc[responseHash] = { + value: + acc[instance.id]?.['responseHashes'][responseHash] ?? responseHash, + count: + (responses_acc[responseHash]?.count ?? 0) + parseInt(count), + }; + return responses_acc;📝 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.responses_acc[responseHash] = { value: acc[instance.id]?.['responseHashes'][responseHash] ?? responseHash, count: (responses_acc[responseHash]?.count ?? 0) + parseInt(count), }; return responses_acc;🧰 Tools
🪛 Biome
[error] 135-135: Avoid the use of spread (
...) syntax on accumulators.Spread syntax should be avoided on accumulators (like those in
.reduce) because it causes a time complexity ofO(n^2).
Consider methods such as .splice or .push instead.(lint/performance/noAccumulatingSpread)
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
apps/frontend-manage/src/pages/sessions/[id]/evaluation.tsx (1)
Line range hint
1-500: Consider completing the terminology migration.While the leaderboard has been updated to use "live quiz" terminology, other parts of the file still use "session" terminology (e.g.,
GetSessionEvaluationDocument,sessionEvaluation). This mixed terminology could lead to confusion and maintenance issues.Consider:
- Migrating remaining session-related GraphQL operations to live quiz terminology
- Updating variable names and types consistently
- Updating comments and documentation to reflect the new architecture
Would you like assistance in identifying all the places that need to be updated?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
apps/frontend-manage/src/pages/sessions/[id]/evaluation.tsx(1 hunks)
🔇 Additional comments (1)
apps/frontend-manage/src/pages/sessions/[id]/evaluation.tsx (1)
250-253: Verify data structure compatibility after renaming.
The change from sessionLeaderboard to liveQuizLeaderboard aligns with the migration to the new live quiz architecture. However, we should verify that the data structure remains compatible with the Leaderboard component.
Let's verify the GraphQL schema and component usage:
✅ Verification successful
Based on the search results, I can now provide a definitive response about the data structure compatibility:
The data structure is compatible after renaming to liveQuizLeaderboard
The codebase shows that:
- The
Leaderboardcomponent inpackages/shared-components/src/Leaderboard.tsxaccepts a genericleaderboardprop of typeLeaderboardCombinedEntry[]which includes: id, participantId, username, avatar, score, rank, etc. - The GraphQL schema in
packages/graphql/src/ops.tsconfirms thatliveQuizLeaderboardreturns entries with the same structure: id, participantId, username, avatar, score, rank. - The transition from
sessionLeaderboardtoliveQuizLeaderboardmaintains the same underlying data structure as both use the sameLeaderboardEntrytype.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check the GraphQL schema and component usage for leaderboard data structure changes
# Test 1: Check if the GraphQL schema defines liveQuizLeaderboard with the same structure
rg -A 10 "type.*Query.*{" | rg -A 10 "liveQuizLeaderboard"
# Test 2: Check Leaderboard component prop types
ast-grep --pattern 'interface Props {
$$$
leaderboard: $_
$$$
}'
Length of output: 223
Script:
#!/bin/bash
# Let's try a different approach to verify the data structure compatibility
# Check GraphQL schema for leaderboard related types
rg -l "liveQuizLeaderboard|sessionLeaderboard" --type graphql
# Check the Leaderboard component implementation
rg -l "Leaderboard" --type typescript --type tsx
# Once we find the files, let's examine their content
rg -A 5 "interface.*Leaderboard" --type typescript --type tsx
# Look for GraphQL query definitions
rg -A 5 "query.*Leaderboard" --type graphql --type typescript --type tsx
Length of output: 385
Script:
#!/bin/bash
# Let's try without file type filters since they're not recognized
# Find files containing Leaderboard
fd -e tsx -e ts | xargs rg -l "Leaderboard"
# Once we find the files, let's look for the component definition and its props
rg -A 10 "interface.*Leaderboard|type.*Leaderboard"
# Look for GraphQL query definitions and types
rg -A 10 "query.*{.*liveQuizLeaderboard|type.*{.*liveQuizLeaderboard"
# Check the actual component implementation
rg -A 10 "const.*Leaderboard.*=|function.*Leaderboard.*\("
Length of output: 267417
klicker-uzh
|
||||||||||||||||||||||||||||
| Project |
klicker-uzh
|
| Branch Review |
new-live-quiz-answering
|
| Run status |
|
| Run duration | 11m 12s |
| 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: 5
🧹 Outside diff range and nitpick comments (23)
packages/graphql/src/graphql/ops/QGetUnassignedLiveQuizzes.graphql (1)
2-6: Consider adding pagination for scalability.Without pagination, this query might face performance issues if there are many unassigned live quizzes. Consider adding pagination parameters:
query GetUnassignedLiveQuizzes { - unassignedLiveQuizzes { + unassignedLiveQuizzes(first: $first, after: $after) { id name status + cursor } + pageInfo { + hasNextPage + endCursor + } }packages/graphql/src/graphql/ops/QGetShortnameQuizzes.graphql (1)
1-1: Consider adding query documentation.Adding a description comment would improve maintainability by documenting the query's purpose and usage context.
+# Retrieves quizzes associated with a shortname along with their course information. +# Used in frontend components for quiz discovery and display. query GetShortnameQuizzes($shortname: String!) {apps/frontend-control/src/pages/course/unassigned.tsx (1)
23-24: Consider extracting filter logic for better maintainability.While the logic is correct, consider refactoring to improve maintainability and reusability.
+const RUNNING_QUIZ_STATUS = [PublicationStatus.Published] +const PLANNED_QUIZ_STATUS = [PublicationStatus.Scheduled, PublicationStatus.Draft] + +const filterQuizzesByStatus = (quizzes, statuses) => + quizzes?.filter((quiz) => statuses.includes(quiz.status)) ?? [] + const runningSessions = useMemo(() => { - return dataSessions?.unassignedLiveQuizzes?.filter( - (session) => session.status === PublicationStatus.Published - ) + return filterQuizzesByStatus(dataSessions?.unassignedLiveQuizzes, RUNNING_QUIZ_STATUS) }, [dataSessions]) const plannedSessions = useMemo(() => { - return dataSessions?.unassignedLiveQuizzes?.filter( - (session) => - session.status === PublicationStatus.Scheduled || - session.status === PublicationStatus.Draft - ) + return filterQuizzesByStatus(dataSessions?.unassignedLiveQuizzes, PLANNED_QUIZ_STATUS) }, [dataSessions])Also applies to: 29-32
apps/frontend-control/src/components/sessions/StartModal.tsx (2)
39-58: Consider improving cache update robustnessWhile the cache update logic is functional, consider these improvements:
- Add explicit null check for cache.readQuery result
- Consider using cache.modify for atomic updates
- Simplify the nested ternary operation
Consider this refactoring:
update(cache) { - const data = cache.readQuery({ - query: GetUnassignedLiveQuizzesDocument, - }) - cache.writeQuery({ - query: GetUnassignedLiveQuizzesDocument, - data: { - unassignedLiveQuizzes: - data?.unassignedLiveQuizzes?.map((quiz) => - quiz.id === quizId - ? { - id: quizId, - name: quizName, - status: PublicationStatus.Published, - } - : quiz - ) ?? [], - }, - }) + const existingData = cache.readQuery({ + query: GetUnassignedLiveQuizzesDocument, + }) + + if (!existingData?.unassignedLiveQuizzes) return + + cache.modify({ + fields: { + unassignedLiveQuizzes(existing = []) { + return existing.map((quiz: any) => { + if (quiz.id !== quizId) return quiz + + return { + id: quizId, + name: quizName, + status: PublicationStatus.Published, + __typename: 'LiveQuizMeta', + } + }) + } + } + }) }
72-72: Consider enhancing error handlingWhile the basic error handling is present, consider providing more specific error feedback to users and logging the error details for debugging.
Consider this enhancement:
try { await startLiveQuiz({ variables: { id: quizId }, }) router.push(`/session/${quizId}`) } catch (error) { + console.error('Failed to start live quiz:', error) setStartModalOpen(false) - setErrorToast(true) + setErrorToast(true) + // Consider adding more specific error messaging based on error type + const errorMessage = error instanceof Error ? error.message : 'Unknown error occurred' + // Consider adding a toast message with the specific error }Also applies to: 74-74, 104-104
apps/frontend-control/src/components/sessions/EmbeddingModal.tsx (1)
90-92: Consider adding type safety improvements.While the code works correctly, consider adding TypeScript type guards or assertions to make the type safety more explicit and maintainable.
-if (!element || !element.elementData) return null +interface ElementData { + name: string; + // add other required properties +} + +interface Element { + id: string; + elementData: ElementData; +} + +function isValidElement(element: any): element is Element { + return element && element.elementData && typeof element.elementData.name === 'string'; +} + +if (!isValidElement(element)) return nullapps/frontend-control/src/components/sessions/SessionLists.tsx (1)
Rename component as part of ongoing quiz terminology migration
The codebase shows an active migration from "session" to "quiz" terminology, with many files already using the new terminology. In this file, rename:
SessionListscomponent toLiveQuizListsSessionListsPropsinterface toLiveQuizListsPropsrunningSessionsprop torunningQuizzesplannedSessionsprop toplannedQuizzesUpdate the translation keys in the i18n message files:
control.course.runningSessionstocontrol.course.runningQuizzescontrol.course.plannedSessionstocontrol.course.plannedQuizzes🔗 Analysis chain
Line range hint
1-142: Consider comprehensive renaming as part of architecture migration.As this is part of a larger architectural shift from sessions to live quizzes, consider renaming other session-related elements for consistency:
- Component name (
SessionLists)- Interface name (
SessionListsProps)- Props (
runningSessions,plannedSessions)- Translation keys (
control.course.runningSessions,control.course.plannedSessions)Let's verify the scope of this migration:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for session-related terminology that might need updating echo "Files still using 'session' terminology:" rg -l 'session' --type tsx --type ts echo "\nFiles already migrated to 'quiz' terminology:" rg -l 'quiz' --type tsx --type tsLength of output: 334
Script:
#!/bin/bash # Search for session-related terminology that might need updating echo "Files still using 'session' terminology:" rg -l "session" echo -e "\nFiles already migrated to 'quiz' terminology:" rg -l "quiz" echo -e "\nTranslation keys with 'session':" rg "control\.course\.(running|planned)Sessions" -A 1 echo -e "\nComponent and interface names with 'Session':" ast-grep --pattern 'interface SessionListsProps' ast-grep --pattern 'function SessionLists'Length of output: 17106
apps/frontend-pwa/src/pages/join/[shortname].tsx (2)
34-43: Simplify the conditional checksThe current null checks have redundant conditions. The check
data.shortnameQuizzes.length === 0is redundant when we already check!data.shortnameQuizzes?.length.Consider simplifying the conditions:
if ( isInactive || !data || - !data.shortnameQuizzes?.length || - data.shortnameQuizzes.length === 0 + !data.shortnameQuizzes?.length ) {
Line range hint
110-130: Consider enhancing error messages for better UXThe server-side logic correctly handles the quiz data and redirects. However, consider adding more descriptive error messages when the shortname is invalid or no quizzes are found.
Example enhancement:
if (!result?.data?.shortnameQuizzes) { return { props: { isInactive: true, + errorMessage: `No quizzes found for instructor ${ctx.params.shortname}`, }, } }packages/graphql/src/services/sessions.ts (5)
Line range hint
15-37: Consider using a Redis key pattern constant.Redis key patterns are currently string literals. Consider extracting these patterns into constants to prevent typos and improve maintainability.
const REDIS_KEY_PATTERNS = { SESSION_LEADERBOARD: (sessionId: string) => `s:${sessionId}:lb`, BLOCK_LEADERBOARD: (sessionId: string, blockId: number) => `s:${sessionId}:b:${blockId}:lb`, INSTANCE_RESPONSE_HASHES: (sessionId: string, instanceId: number) => `s:${sessionId}:i:${instanceId}:responseHashes`, // ... other patterns } as const;
Line range hint
39-89: Improve type safety in processCachedData function.The function uses 'any' types extensively, which reduces type safety. Consider defining proper interfaces for the cached results and return types.
interface CachedResult { responseHashes: Record<string, string>; responses: Record<string, string>; results: Record<string, { count: number; value: string; }>; participants: number; } interface ProcessedData { sessionLeaderboard: Record<string, string>; blockLeaderboard: Record<string, string>; cachedResults: CachedResult[]; instanceResults: Record<string, CachedResult>; }
Line range hint
91-186: Avoid mutating input parameters.The
checkCorrectnessFreeTextandcomputeStatisticsfunctions mutate their input parameters. Consider returning new objects instead for better predictability and easier testing.function checkCorrectnessFreeText(instance: PickedInstanceType): PickedInstanceType { if (instance.elementType !== 'FREE_TEXT' || instance.questionData.type !== 'FREE_TEXT') { return instance; } return { ...instance, results: Object.fromEntries( Object.entries(instance.results).map(([id, result]) => [ id, { ...result, correct: instance.questionData.options.solutions ? instance.questionData.options.solutions .map((s: string) => s.toLowerCase()) .includes(result.value.toLowerCase()) : undefined } ]) ) }; }
Line range hint
188-365: Consider splitting getSessionEvaluation into smaller functions.The function handles multiple responsibilities including authentication, data fetching, and processing. Consider splitting it into smaller, focused functions for better maintainability and testing.
Example structure:
async function validateSessionAccess(id: string, hmac: string | null | undefined, user?: Context['user']): Promise<boolean>; async function fetchSessionData(id: string, user?: Context['user']): Promise<Session | null>; async function processActiveBlock(session: Session, ctx: Context): Promise<ProcessedBlock>; async function formatSessionResponse(session: Session, activeBlock?: ProcessedBlock): Promise<SessionEvaluation>;Also, consider adding more specific error handling:
if (!process.env.APP_SECRET) { throw new Error('APP_SECRET environment variable is not set'); }
Line range hint
271-282: Strengthen security measures in session validation.The HMAC validation is critical for security, but there are areas for improvement:
- APP_SECRET is used without validation
- The HMAC comparison might be vulnerable to timing attacks
Consider these improvements:
import { timingSafeEqual } from 'crypto'; // Validate environment variable at startup if (!process.env.APP_SECRET || process.env.APP_SECRET.length < 32) { throw new Error('APP_SECRET must be set and at least 32 characters long'); } // Use timing-safe comparison const expectedHmac = Buffer.from(sessionHmac); const providedHmac = Buffer.from(hmac); if (!timingSafeEqual(expectedHmac, providedHmac)) { return null; }cypress/cypress/support/commands.ts (2)
Line range hint
633-633: Address TODO comment regarding blocks vs stacks migration.The TODO comment "update this to create blocks instead of stacks" suggests an incomplete migration. This inconsistency between blocks and stacks terminology should be resolved.
Consider:
- Documenting the architectural decision to use either blocks or stacks consistently
- Creating a migration plan to update all quiz types to use the same terminology
- Updating the comment to include a ticket reference or timeline for the migration
Line range hint
633-636: Inconsistent terminology between quiz types.The codebase shows mixed usage of blocks and stacks:
createLiveQuizuses blockscreatePracticeQuizandcreateMicroLearninguse stacks
This inconsistency could lead to maintenance issues and confusion.Consider standardizing the terminology and data structures across all quiz types to improve maintainability and reduce cognitive load.
packages/graphql/src/services/courses.ts (1)
644-644: Consider adding a TypeScript interface for the return type.While the implementation is correct, defining an interface for the return type would improve type safety and documentation.
+interface CourseSummary { + numOfParticipations: number; + numOfLiveQuizzes: number; + numOfPracticeQuizzes: number; + numOfMicroLearnings: number; + numOfGroupActivities: number; + numOfLeaderboardEntries: number; + numOfParticipantGroups: number; +} export async function getCourseSummary( { courseId }: { courseId: string }, - ctx: ContextWithUser + ctx: ContextWithUser ): Promise<CourseSummary | null> {packages/graphql/src/services/liveQuizzes.ts (3)
1140-1149: Enhance error handling indeactivateLiveQuizBlock.The error handling could be more specific and provide better context for debugging.
Consider adding specific error types and more context:
- } catch (error: any) { + } catch (error) { + const errorContext = { + quizId, + blockId, + activeBlockId: quiz.activeBlockId, + error: error instanceof Error ? error.message : String(error) + } await sendTeamsNotifications( 'graphql/deactivateLiveQuizBlock', - `ERROR - failed to deactivate block ${blockId} in live quiz ${ - quiz.id - } with active block ${quiz.activeBlockId}: ${error?.message || error}` + `ERROR - failed to deactivate block: ${JSON.stringify(errorContext)}` ) throw error }
1934-1940: Optimize block order calculation.The current implementation uses
Math.maxwith spread operator which could be inefficient for large arrays.Consider using reduce for better performance:
- const lastBlockOrder = executedBlockOrders - ? Math.max(...executedBlockOrders) - : 0 + const lastBlockOrder = executedBlockOrders?.reduce( + (max, order) => (order > max ? order : max), + 0 + ) ?? 0
Line range hint
765-786: Use optional chaining for better readability.The code uses multiple conditional checks that could be simplified with optional chaining.
Apply this change:
- if (liveQuiz.activeBlock && liveQuiz.activeBlock.id) { + if (liveQuiz.activeBlock?.id) { const activeInstanceIds = liveQuiz.activeBlock?.elements.map( (instance) => instance.id )🧰 Tools
🪛 Biome
[error] 765-765: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
packages/graphql/src/public/server.json (1)
131-131: Consider pagination for large leaderboards.The query looks good but consider implementing pagination for better performance when dealing with large numbers of participants.
Consider:
- Adding pagination parameters (limit/offset or cursor-based)
- Implementing caching for leaderboard data
- Using database indexing for efficient sorting by score
packages/graphql/src/ops.schema.json (2)
Line range hint
14270-14321: LGTM: Good separation of concerns between quiz and microlearning endpoints.The mutations maintain consistent patterns while properly separating different learning formats.
Consider documenting the differences between LiveQuiz and MicroLearning in the schema descriptions to help API consumers understand when to use each type.
Line range hint
21800-21877: LGTM: The quiz queries are well-structured and purposeful.The queries follow consistent patterns and have clear, descriptive names.
Consider adding descriptions to these queries to document their specific use cases and the concept of "unassigned" quizzes.
🛑 Comments failed to post (5)
apps/frontend-control/src/components/sessions/SessionLists.tsx (1)
112-113: 🛠️ Refactor suggestion
Maintain consistent terminology throughout the component.
While the props have been updated to use "quiz" terminology (
quizId,quizName), the rest of the component, including variable names (startId,startName), still uses "session" terminology. This inconsistency could lead to confusion and maintenance issues.Consider applying these changes for consistency:
- const [startId, setStartId] = useState('') - const [startName, setStartName] = useState('') + const [quizId, setQuizId] = useState('') + const [quizName, setQuizName] = useState('') // Update the onClick handler accordingly onClick={() => { setStartModalOpen(true) - setStartId(session.id) - setStartName(session.name) + setQuizId(session.id) + setQuizName(session.name) }}Committable suggestion skipped: line range outside the PR's diff.
apps/frontend-control/src/pages/session/[id].tsx (1)
157-163: 🛠️ Refactor suggestion
Consider improving error handling for block activation.
The fallback value of -1 for
blockIdcould cause runtime issues if the block is not found. Consider adding proper error handling or validation.- blockId: - blocks.find((block) => block.order === currentBlockOrder) - ?.id || -1, + blockId: blocks.find((block) => block.order === currentBlockOrder)?.id, + onError: (error) => { + // Handle the error appropriately + console.error('Failed to activate block:', error) + },Committable suggestion skipped: line range outside the PR's diff.
packages/graphql/src/services/sessions.ts (1)
11-13: 🛠️ Refactor suggestion
Move achievement IDs to configuration.
The achievement IDs are currently hardcoded with a FIXME comment. These should be moved to a configuration file or environment variables for better maintainability.
Consider creating an enum or configuration object for achievements:
// config/achievements.ts export const AchievementIds = { FIRST: 5, SECOND: 6, THIRD: 7, } as const;packages/graphql/src/services/liveQuizzes.ts (2)
885-896:
⚠️ Potential issuePotential memory leak in scheduled jobs.
The scheduled jobs are stored in a global object but might not be properly cleaned up if the server restarts or crashes.
Consider:
- Using a persistent job queue system
- Adding cleanup logic on server shutdown
- Implementing job recovery on server restart
+ // TODO: Consider using a proper job queue system like Bull scheduledJobs[blockId] = schedule.scheduleJob( dayjs(updatedQuiz.activeBlock.expiresAt).add(20, 'second').toDate(), async () => { + try { await deactivateLiveQuizBlock({ quizId, blockId }, ctx, true) ctx.emitter.emit('invalidate', { typename: 'LiveQuiz', id: updatedQuiz.id, }) + } catch (error) { + console.error(`Failed to deactivate block ${blockId}:`, error) + } finally { + delete scheduledJobs[blockId] + } } )📝 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 (updatedQuiz.activeBlock?.expiresAt) { // TODO: Consider using a proper job queue system like Bull scheduledJobs[blockId] = schedule.scheduleJob( dayjs(updatedQuiz.activeBlock.expiresAt).add(20, 'second').toDate(), async () => { try { await deactivateLiveQuizBlock({ quizId, blockId }, ctx, true) ctx.emitter.emit('invalidate', { typename: 'LiveQuiz', id: updatedQuiz.id, }) } catch (error) { console.error(`Failed to deactivate block ${blockId}:`, error) } finally { delete scheduledJobs[blockId] } } ) }
91-186: 🛠️ Refactor suggestion
Performance optimization needed in
processCachedDatareducer.The reducer implementation uses spread operators in accumulator objects, which can lead to O(n²) time complexity as the data grows.
Consider using direct property assignment instead:
- return { - ...acc, - [instance.id]: { - ...acc[instance.id], - anonymousResults, - }, - } + acc[instance.id] = { + ...acc[instance.id], + anonymousResults, + } + return acc📝 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.> = mappedResults.slice(2).reduce((acc, cacheObj, ix) => { const ixMod = ix % 3 const instance = activeBlock.elements[Math.floor((ix - ixMod) / 3)] if (!instance) return acc switch (ixMod) { // compute element instance results from cache entries case 2: { // TODO: if possible, split up results and anonymous results here (potentially the cache content needs to augmented) let anonymousResults: | ElementResultsChoices | ElementResultsOpen | ElementResultsFlashcard | ElementResultsContent | undefined if ( instance.elementType === ElementType.SC || instance.elementType === ElementType.MC || instance.elementType === ElementType.KPRIM ) { const choices = Object.entries( omitBy(cacheObj, (_, key) => key === 'participants') ).reduce<Record<string, number>>((acc, [responseHash, count]) => { return { ...acc, [responseHash]: (acc[responseHash] ?? 0) + parseInt(count), } }, {}) anonymousResults = { choices, total: parseInt(cacheObj.participants), } as ElementResultsChoices } else if ( instance.elementType === ElementType.NUMERICAL || instance.elementType === ElementType.FREE_TEXT ) { const responses = Object.entries( omitBy(cacheObj, (_, key) => key === 'participants') ).reduce<Record<string, { value: string; count: number }>>( (responses_acc, [responseHash, count]) => { return { ...responses_acc, [responseHash]: { value: acc[instance.id]?.['responseHashes'][responseHash] ?? responseHash, count: (responses_acc[responseHash]?.count ?? 0) + parseInt(count), }, } }, {} ) anonymousResults = { responses, total: parseInt(cacheObj.participants), } as ElementResultsOpen } acc[instance.id] = { ...acc[instance.id], anonymousResults, } return acc } // responses case 1: acc[instance.id] = { ...acc[instance.id], responses: cacheObj, } return acc // response hashes case 0: acc[instance.id] = { ...acc[instance.id], responseHashes: cacheObj, } return acc default: return acc } }, {})🧰 Tools
🪛 Biome
[error] 117-117: Avoid the use of spread (
...) syntax on accumulators.Spread syntax should be avoided on accumulators (like those in
.reduce) because it causes a time complexity ofO(n^2).
Consider methods such as .splice or .push instead.(lint/performance/noAccumulatingSpread)
[error] 135-135: Avoid the use of spread (
...) syntax on accumulators.Spread syntax should be avoided on accumulators (like those in
.reduce) because it causes a time complexity ofO(n^2).
Consider methods such as .splice or .push instead.(lint/performance/noAccumulatingSpread)
[error] 155-155: Avoid the use of spread (
...) syntax on accumulators.Spread syntax should be avoided on accumulators (like those in
.reduce) because it causes a time complexity ofO(n^2).
Consider methods such as .splice or .push instead.(lint/performance/noAccumulatingSpread)
[error] 166-166: Avoid the use of spread (
...) syntax on accumulators.Spread syntax should be avoided on accumulators (like those in
.reduce) because it causes a time complexity ofO(n^2).
Consider methods such as .splice or .push instead.(lint/performance/noAccumulatingSpread)
[error] 176-176: Avoid the use of spread (
...) syntax on accumulators.Spread syntax should be avoided on accumulators (like those in
.reduce) because it causes a time complexity ofO(n^2).
Consider methods such as .splice or .push instead.(lint/performance/noAccumulatingSpread)
Summary by CodeRabbit
Release Notes
New Features
LiveQuizLeaderboardanduseSingleStudentResponse.UI Updates
Localization Changes
Refactor