refactor: implement functionalities for live quiz interaction besides question answering workflow #4352
Conversation
📝 WalkthroughWalkthroughThis pull request implements extensive changes across various components and services, primarily transitioning from session-based logic to quiz-based logic. Key modifications include renaming identifiers from 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: 19
🧹 Outside diff range and nitpick comments (44)
packages/graphql/src/graphql/ops/SRunningSessionUpdated.graphql (1)
1-13: Consider adding subscription documentation.To improve maintainability, consider adding a documentation comment describing:
- The purpose of this subscription
- When it triggers
- The meaning of each field
Add this documentation above the subscription:
+# Subscribes to updates of a running live quiz +# Triggers when: +# - Quiz status changes +# - Time limit is updated +# - Elements are modified +# - Execution parameters change subscription RunningLiveQuizUpdated($quizId: String!) {apps/frontend-pwa/src/components/microLearning/MicroLearningSubscriber.tsx (1)
Line range hint
18-48: Consider adding explicit subscription cleanupWhile the current implementation works, it would be better to explicitly clean up the subscription when the component unmounts.
Here's how you could improve it:
useEffect(() => { - subscribeToMore({ + const unsubscribe = subscribeToMore({ document: MicroLearningEndedDocument, variables: { activityId }, updateQuery: ( prev: { microLearning: MicroLearning }, { subscriptionData, }: { subscriptionData: { data: { microLearningEnded: MicroLearning } } } ): { microLearning: MicroLearning } => { if (!subscriptionData.data) return prev // trigger toast for ended microlearning setEndedMicroLearning(true) // update the values returned by the course overview data query const updatedMicroLearning = { ...prev.microLearning, ...subscriptionData.data.microLearningEnded, } return { microLearning: updatedMicroLearning } }, }) + return () => { + if (typeof unsubscribe === 'function') { + unsubscribe() + } + } }, [activityId, subscribeToMore])apps/frontend-pwa/src/components/groupActivity/GroupActivitySubscriber.tsx (2)
11-11: Consider further improving type safety.While changing from
anyto(doc: SubscribeToMoreOptions) => anyis a good improvement, we could make it even more type-safe by specifying the return type.- subscribeToMore: (doc: SubscribeToMoreOptions) => any + subscribeToMore: (doc: SubscribeToMoreOptions) => () => voidThis explicitly shows that the function returns an unsubscribe callback.
Line range hint
20-45: Consider adding error handling for the subscription.The subscription setup should handle potential errors to ensure robustness in the live quiz interaction workflow.
useEffect(() => { subscribeToMore({ document: SingleGroupActivityEndedDocument, variables: { activityId }, + onError: (error) => { + console.error('Error in group activity subscription:', error) + // Consider adding error handling logic or notification + }, updateQuery: ( prev: { groupActivityDetails: GroupActivityDetails }, {apps/frontend-pwa/src/components/microLearning/MicroLearningListSubscriber.tsx (3)
Line range hint
1-13: Consider further improving type safety ofsubscribeToMore.While the change from
anyto a specific function type is a good improvement, we can make it even more type-safe by specifying the return type.Consider updating the type to:
- subscribeToMore: (doc: SubscribeToMoreOptions) => any + subscribeToMore: (doc: SubscribeToMoreOptions) => () => voidThis explicitly shows that the function returns an unsubscribe function, which is the actual return type from Apollo's
subscribeToMore.
Line range hint
20-21: Add cleanup function to prevent memory leaks.The
useEffecthook should return a cleanup function to unsubscribe when the component unmounts.Update the effect to include cleanup:
useEffect(() => { - subscribeToMore({ + const unsubscribe = subscribeToMore({ document: MicroLearningEndedDocument, // ... rest of the subscription config }) + return () => { + if (unsubscribe) unsubscribe() + } }, [activityId, subscribeToMore])
Line range hint
33-65: Simplify the participation update logic.The nested mapping and filtering logic is complex and could be simplified for better readability and maintainability.
Consider refactoring the update logic:
- const updatedParticipations: Participation[] = prev.participations.map( - (participation) => { - const microLearningIds = participation.course?.microLearnings?.map( - (ml) => ml.id - ) - if (microLearningIds?.includes(activityId)) { - return { - ...participation, - course: { - ...participation.course, - microLearnings: - participation.course?.microLearnings - ?.map((ml) => { - if (ml.id === activityId) { - return undefined - } - return ml - }) - .filter((ml) => typeof ml !== 'undefined') ?? [], - } as Course, - } - } - return participation - } - ) + const updatedParticipations = prev.participations.map((participation) => { + if (!participation.course?.microLearnings?.some(ml => ml.id === activityId)) { + return participation + } + + return { + ...participation, + course: { + ...participation.course, + microLearnings: participation.course.microLearnings?.filter( + (ml) => ml.id !== activityId + ) ?? [], + } as Course, + } + })This refactoring:
- Simplifies the logic by using
somefor existence check- Directly filters out the ended micro-learning
- Maintains the same functionality with improved readability
apps/frontend-pwa/src/components/groupActivity/GroupActivityListSubscriber.tsx (3)
1-1: Consider further improving type safety.While changing
subscribeToMorefromanyto a typed function is a good improvement, we could make it even more type-safe by:
- Specifying the return type instead of using
any- Making it generic to handle different subscription data types
Consider this improvement:
- subscribeToMore: (doc: SubscribeToMoreOptions) => any + subscribeToMore: <TSubscriptionData = any>( + doc: SubscribeToMoreOptions<any, any, TSubscriptionData> + ) => () => voidAlso applies to: 11-11
Line range hint
22-45: Add duplicate protection to the "ended" subscription.The "started" subscription has protection against duplicate triggers, but the "ended" subscription doesn't. This could lead to unnecessary updates and UI flickers.
Add similar protection:
if (!subscriptionData.data) return prev + // Check for duplicate updates + const updatedActivity = subscriptionData.data.groupActivityEnded + if ( + prev.groupActivities.some( + (activity) => + activity.id === updatedActivity.id && + activity.endedAt === updatedActivity.endedAt + ) + ) { + return prev + } + // trigger toast for ended group activity - const updatedActivity = subscriptionData.data.groupActivityEnded setEndedGroupActivity(updatedActivity.displayName)
Line range hint
15-89: Consider adding error handling and loading states.The component would benefit from proper error handling for subscription failures and exposing loading states.
Consider these improvements:
- Add error handling:
const [error, setError] = useState<Error | null>(null); useEffect(() => { try { const unsubscribe1 = subscribeToMore({ // ... existing config onError: (error) => { setError(error); console.error('Subscription error:', error); }, }); const unsubscribe2 = subscribeToMore({ // ... existing config onError: (error) => { setError(error); console.error('Subscription error:', error); }, }); return () => { unsubscribe1(); unsubscribe2(); }; } catch (error) { setError(error as Error); console.error('Subscription setup error:', error); } }, [courseId, subscribeToMore]);
- Expose error state:
if (error) { return <div>Error: {error.message}</div>; }apps/frontend-pwa/src/components/liveSession/FeedbackAreaSubscriber.tsx (4)
10-16: Add JSDoc documentation and improve type safety.Consider adding JSDoc documentation for the component and making the return type of
subscribeToMoremore specific thanany.+/** + * Subscribes to real-time feedback updates for a quiz + * @param {Object} props - Component props + * @param {string} props.quizId - Unique identifier of the quiz + * @param {Function} props.subscribeToMore - Apollo Client's subscribeToMore function + */ function FeedbackAreaSubscriber({ subscribeToMore, quizId, }: { quizId: string - subscribeToMore: (doc: SubscribeToMoreOptions) => any + subscribeToMore: (doc: SubscribeToMoreOptions) => () => void })
57-76: LGTM with a minor suggestion.The update logic is correct. Consider using immutability helpers like Immer for more readable state updates.
+import produce from 'immer' updateQuery: ( prev: { feedbacks: Feedback[] }, { subscriptionData, }: { subscriptionData: { data: { feedbackUpdated: Feedback } } } ) => { if (!subscriptionData.data) return prev const updatedItem = subscriptionData.data.feedbackUpdated - return { - ...prev, - feedbacks: prev.feedbacks?.map((item) => { - if (item.id === updatedItem.id) return updatedItem - return item - }), - } + return produce(prev, (draft) => { + const index = draft.feedbacks.findIndex((item) => item.id === updatedItem.id) + if (index !== -1) draft.feedbacks[index] = updatedItem + }) }
78-83: Use optional chaining for cleaner cleanup.The static analysis tool correctly suggests using optional chaining for a more concise cleanup.
return () => { - feedbackAdded && feedbackAdded() - feedbackRemoved && feedbackRemoved() - feedbackUpdated && feedbackUpdated() + feedbackAdded?.() + feedbackRemoved?.() + feedbackUpdated?.() }🧰 Tools
🪛 Biome
[error] 79-79: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 80-80: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 81-81: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
85-88: Return null instead of an empty div.Since this is a subscription-only component that doesn't render anything, return
nullinstead of an empty div to avoid creating unnecessary DOM nodes.- return <div></div> + return nullpackages/graphql/src/schema/subscription.ts (1)
39-50: Consider adding JSDoc comments for the feedback subscriptionsThe changes look good, but adding documentation would help clarify the purpose and usage of these subscriptions, especially given the architectural changes.
+/** + * Subscription for newly created feedback in a live quiz. + * @param quizId - The ID of the live quiz to subscribe to + * @returns The newly created feedback + */ feedbackCreated: t.field({apps/frontend-manage/src/components/interaction/feedbacks/FeedbackChannel.tsx (3)
110-112: Consider consolidating spacing approach.While the flex layout classes improve alignment, consider using either gap-based or margin-based spacing consistently across similar components for better maintainability.
- className={{ - root: 'flex h-9 w-9 items-center justify-center', - }} + className={{ + root: 'flex h-9 w-9 items-center justify-center gap-2', + }}
119-121: Consider adding ARIA labels for accessibility.The icon implementation is cleaner, but consider adding aria-labels to improve accessibility for screen readers.
- <FontAwesomeIcon icon={faEye} /> + <FontAwesomeIcon icon={faEye} aria-label="Make feedback visible" /> - <FontAwesomeIcon icon={faEyeSlash} /> + <FontAwesomeIcon icon={faEyeSlash} aria-label="Hide feedback" />
Line range hint
1-1: Address or remove TODO comments.There are several TODO comments in the file that should be addressed or removed before merging:
- Line 1: "TODO: notifications"
- Line 73: "TODO: styling"
- Line 82: "TODO: styling"
Would you like me to help implement any of these TODOs or create GitHub issues to track them?
packages/graphql/src/schema/liveQuiz.ts (1)
Line range hint
81-91: Consider adding JSDoc documentation.Since this interface is now part of the public API, it would be beneficial to add JSDoc documentation describing its purpose and usage.
+/** + * Represents a live quiz with its associated blocks, feedbacks, and settings. + * Used for managing the state and interactions of an active quiz session. + */ export interface ILiveQuiz extends DB.LiveQuiz {apps/frontend-pwa/src/components/liveSession/PublicFeedback.tsx (1)
11-23: LGTM! Consider extracting type definition.The explicit Promise return types and inline interface definition improve type safety and clarity. However, if these types are used elsewhere, consider extracting them into a separate types file for reusability.
// types.ts export interface FeedbackHandlers { onUpvoteFeedback: (id: number, change: number) => Promise<void> onReactToFeedbackResponse: ( id: number, upvoteChange: number, downvoteChange: number ) => Promise<void> }packages/graphql/src/schema/course.ts (1)
28-29: Track technical debt: Add more context to the TODO comment.While the TODO comment indicates future removal of sessions, it would be helpful to:
- Add more context about the migration plan
- Link to a tracking issue or document
The addition of
liveQuizzesproperty is well-structured and maintains consistency with other array properties in the interface.apps/frontend-pwa/src/pages/session/[id].tsx (2)
Line range hint
92-92: Update sessionId to quizId in handleNewResponse.The request body still uses
sessionIdwhile the codebase is transitioning to quiz-based naming.Apply this diff to fix the request body:
body: JSON.stringify({ instanceId: instanceId, - sessionId: id, + quizId: id, response: { choices: answer }, }),
Line range hint
214-214: Update component props to use quizId.The following components still use
sessionIdprops:
- SessionLeaderboard component
- QuestionArea component
This should be updated to maintain consistency with the quiz-based naming convention.
Apply these changes:
- Update SessionLeaderboard usage:
-<SessionLeaderboard sessionId={id} /> +<SessionLeaderboard quizId={id} />
- Update QuestionArea usage:
-sessionId={id} +quizId={id}Also applies to: 255-255
apps/frontend-manage/src/components/interaction/AudienceInteraction.tsx (4)
Line range hint
82-88: Consider grouping related mutation hooks.While the mutation hooks are correctly updated, consider grouping them by functionality for better maintainability:
- const [changeQuizSettings] = useMutation(ChangeLiveQuizSettingsDocument) - const [publishFeedback] = useMutation(PublishFeedbackDocument) - const [pinFeedback] = useMutation(PinFeedbackDocument) - const [resolveFeedback] = useMutation(ResolveFeedbackDocument) - const [deleteFeedback] = useMutation(DeleteFeedbackDocument) - const [deleteFeedbackResponse] = useMutation(DeleteFeedbackResponseDocument) - const [respondToFeedback] = useMutation(RespondToFeedbackDocument) + // Quiz settings mutation + const [changeQuizSettings] = useMutation(ChangeLiveQuizSettingsDocument) + + // Feedback management mutations + const [publishFeedback] = useMutation(PublishFeedbackDocument) + const [pinFeedback] = useMutation(PinFeedbackDocument) + const [resolveFeedback] = useMutation(ResolveFeedbackDocument) + + // Feedback response mutations + const [respondToFeedback] = useMutation(RespondToFeedbackDocument) + const [deleteFeedbackResponse] = useMutation(DeleteFeedbackResponseDocument) + const [deleteFeedback] = useMutation(DeleteFeedbackDocument)
Line range hint
121-146: Consider adding error handling for quiz settings mutations.While the mutation handlers are correctly updated, they lack error handling which could lead to a poor user experience if the mutations fail.
onCheckedChange={(): void => { changeQuizSettings({ variables: { id: quizId, isLiveQAEnabled: !isLiveQAEnabled, }, + onError: (error) => { + // Handle error appropriately (e.g., show toast notification) + console.error('Failed to update quiz settings:', error) + } }) push([ 'trackEvent', 'Running Session', !isLiveQAEnabled ? 'Feedback Channel Activated' : 'Feedback Channel Deactivated', ]) }}
Line range hint
255-283: Consider adding loading states for feedback mutations.The feedback mutation handlers have been simplified, but they should handle loading states to provide better user feedback during operations.
handlePinFeedback={(feedbackId: number, isPinned: boolean) => { pinFeedback({ variables: { id: feedbackId, isPinned }, + onCompleted: () => { + // Handle success (e.g., show success toast) + }, + onError: (error) => { + // Handle error (e.g., show error toast) + } }) push([ 'trackEvent', 'Running Session', 'Feedback Pinned', String(isPinned), ]) }}Similar improvements can be applied to
publishFeedbackandresolveFeedbackhandlers.
Line range hint
323-334: Consider adding loading state to confusion feedback toggle.The confusion feedback toggle should indicate when the setting is being updated.
<Switch data={{ cy: 'toggle-gamification' }} checked={isConfusionFeedbackEnabled} + disabled={loading} onCheckedChange={(): void => { changeQuizSettings({ variables: { id: quizId, isConfusionFeedbackEnabled: !isConfusionFeedbackEnabled, }, + onCompleted: () => { + // Handle success + }, + onError: (error) => { + // Handle error + } }) push([ 'trackEvent', 'Running Session', 'Feedback Moderation Toggled', String(!isConfusionFeedbackEnabled), ]) }}packages/graphql/src/public/client.json (1)
Line range hint
1-179: Consider documenting the breaking changes.Given the significant transition from session-based to quiz-based logic, it would be helpful to document these changes in the GraphQL schema documentation or README to help API consumers adapt their implementations.
packages/graphql/src/schema/query.ts (1)
163-163: Consider adding JSDoc comments for better documentation.Adding JSDoc comments would improve the documentation by clarifying the purpose and usage of the
feedbacksfield.+ /** + * Retrieves feedbacks for a specific quiz + * @param quizId - The ID of the quiz to fetch feedbacks for + * @returns An array of Feedback objects associated with the quiz + */ feedbacks: t.field({ nullable: true, type: [Feedback], args: { quizId: t.arg.string({ required: true }), },packages/graphql/src/services/participants.ts (1)
123-124: Consider adding isDeleted filter for consistency.The inclusion of
liveQuizzeswithPublicationStatus.PUBLISHEDfilter looks good. However, for consistency with themicroLearningsfilter above, consider also filtering out deleted quizzes.liveQuizzes: { - where: { status: PublicationStatus.PUBLISHED }, + where: { + status: PublicationStatus.PUBLISHED, + isDeleted: false + }, },packages/graphql/src/ops.schema.json (1)
Line range hint
24397-24419: LGTM: New subscription supports real-time quiz updates.The
runningLiveQuizUpdatedsubscription is well-structured for real-time quiz state updates.Consider implementing proper error handling in the subscription resolver to handle scenarios like:
- Quiz not found
- Quiz ended
- Connection issues
This will ensure a better real-time experience for users.apps/frontend-pwa/src/lib/hooks/useStudentOverviewSplit.tsx (3)
82-93: Refactor duplicated code when constructing course objects.The code blocks for constructing
courses(lines 82-93) andoldCourses(lines 99-110) are nearly identical, leading to code duplication. Consider extracting the shared logic into a helper function to improve maintainability and reduce redundancy.You can create a helper function to map a participation to a
LocalCourseType:function mapParticipationToCourse(participation: Participation): LocalCourseType { return { id: participation.course?.id, displayName: participation.course?.displayName, startDate: participation.course?.startDate, endDate: participation.course?.endDate, isGamificationEnabled: participation.course?.isGamificationEnabled, isSubscribed: participation.subscriptions?.length > 0 ?? false, } }Then, update the assignments in the reducer:
courses: (dayjs(course.endDate).isAfter(dayjs()) || dayjs(course.endDate).isSame(dayjs())) - ? [ - ...acc.courses, - { - id: course.id, - displayName: course.displayName, - startDate: course.startDate, - endDate: course.endDate, - isGamificationEnabled: course.isGamificationEnabled, - isSubscribed: - (participation.subscriptions && participation.subscriptions.length > 0) ?? - false, - }, - ] + ? [...acc.courses, mapParticipationToCourse(participation)] : acc.courses, oldCourses: dayjs(course.endDate).isBefore(dayjs()) - ? [ - ...acc.oldCourses, - { - id: course.id, - displayName: course.displayName, - startDate: course.startDate, - endDate: course.endDate, - isGamificationEnabled: course.isGamificationEnabled, - isSubscribed: - (participation.subscriptions && participation.subscriptions.length > 0) ?? - false, - }, - ] + ? [...acc.oldCourses, mapParticipationToCourse(participation)] : acc.oldCourses,Also applies to: 99-110
89-92: Simplify the logic for the 'isSubscribed' property.Currently, the
isSubscribedproperty uses a combination of logical operators and the nullish coalescing operator, which can be simplified. The expression(participation.subscriptions && participation.subscriptions.length > 0) ?? falsecan be refactored for clarity.Apply this diff to simplify the logic:
- isSubscribed: - (participation.subscriptions && - participation.subscriptions.length > 0) ?? - false, + isSubscribed: participation.subscriptions?.length > 0 ?? false,This change makes use of optional chaining and directly evaluates to a boolean value. Apply this change to both the
coursesandoldCoursessections.Also applies to: 107-110
115-120: Optimize array concatenation for quizzes and micro-learnings.When constructing
activeLiveQuizzesandactiveMicrolearning, you're using the spread operator to concatenate arrays, including an empty array when there are no items. This can be optimized by conditionally spreading the arrays only when they contain elements.Modify the code as follows:
activeLiveQuizzes: [ ...acc.activeLiveQuizzes, - ...(course.liveQuizzes?.map((quiz) => ({ + ...course.liveQuizzes?.map((quiz) => ({ id: quiz.id, displayName: quiz.displayName, courseName: course.displayName, - })) ?? []), + })) ?? [], ], activeMicrolearning: [ ...acc.activeMicrolearning, - ...(course.microLearnings?.map((micro) => ({ + ...course.microLearnings?.map((micro) => ({ id: micro.id, displayName: micro.displayName, scheduledStartAt: micro.scheduledStartAt, scheduledEndAt: micro.scheduledEndAt, courseName: course.displayName, isCompleted: participation.completedMicroLearnings?.includes( micro.id ), - })) ?? []), + })) ?? [], ],By removing the unnecessary empty arrays, you prevent potential performance overhead.
Also applies to: 123-133
packages/graphql/src/services/feedbacks.ts (6)
71-71: Improve Error Handling by Throwing Specific ExceptionsReturning
nullwhen the quiz is not found orisLiveQAEnabledisfalsemay not provide sufficient information to the caller. Consider throwing specific errors to indicate the exact issue.Consider modifying the code as follows:
-if (!quiz || !quiz.isLiveQAEnabled) return null +if (!quiz) { + throw new Error('Quiz not found') +} +if (!quiz.isLiveQAEnabled) { + throw new Error('Live Q&A is not enabled for this quiz') +}
149-157: Consistent Parameter Typing for 'addConfusionTimestep' FunctionEnsure that the parameter types are consistently defined. The inline types in the destructured parameters can be extracted into a separate interface for clarity.
Consider defining an interface for the parameters:
interface AddConfusionTimestepArgs { quizId: string difficulty: number speed: number } export async function addConfusionTimestep( { quizId, difficulty, speed }: AddConfusionTimestepArgs, ctx: Context ) { // function body }
266-266: Improve Error Handling in 'resolveFeedback' FunctionReturning
nullwhen the feedback is not found or the user is unauthorized may not provide sufficient context. Throwing specific errors can improve debuggability and user feedback.Modify the code:
-if (!feedback || feedback.liveQuiz!.ownerId !== ctx.user.sub) return null +if (!feedback) { + throw new Error('Feedback not found') +} +if (!feedback.liveQuiz || feedback.liveQuiz.ownerId !== ctx.user.sub) { + throw new Error('Unauthorized access') +}
300-300: Handle Null Cases Explicitly in 'deleteFeedback'To enhance clarity and error handling, explicitly check for
nulland throw meaningful errors instead of returningnull.Revise the condition:
-if (!feedback || feedback.liveQuiz!.ownerId !== ctx.user.sub) return null +if (!feedback) { + throw new Error('Feedback not found') +} +if (!feedback.liveQuiz || feedback.liveQuiz.ownerId !== ctx.user.sub) { + throw new Error('Unauthorized access') +}
65-67: Optimize Database Query by Selecting Only Necessary FieldsThe current query fetches the entire
liveQuizobject, which may include unnecessary data. To improve performance, select only the fields you need.Modify the query:
-const quiz = await ctx.prisma.liveQuiz.findUnique({ - where: { - id: quizId, - }, -}) +const quiz = await ctx.prisma.liveQuiz.findUnique({ + where: { id: quizId }, + select: { id: true, isLiveQAEnabled: true, isModerationEnabled: true }, +})
89-89: Consider Removing Debugging Statements Before Production DeploymentThe
console.logstatement may have been used for debugging purposes. It's good practice to remove such statements to keep the production console clean.-console.log('TRIGGERING FEEDBACK ADDED SUBSCRIPTION')apps/frontend-pwa/src/components/liveSession/FeedbackArea.tsx (3)
108-108: Specify a proper type instead of usinganyforconfusionUsing the
anytype can undermine TypeScript's type safety features. Define an appropriate type forconfusionto improve code maintainability and readability.For example, define an interface:
interface ConfusionData { prevSpeed: number prevDifficulty: number prevTimestamp: string } const confusion: ConfusionData | null = await localForage.getItem<ConfusionData>( `${quizId}-confusion` )
27-41: Refactor color maps to reduce code duplicationThe
RANGE_COLOR_MAPandBORDER_COLOR_MAPconstants have similar structures with overlapping values. Consider refactoring to reduce duplication by using a single source of truth or generating these maps dynamically.For example:
const COLOR_VALUES = { '-2': { bg: 'bg-red-200', border: 'border-red-300' }, '-1': { bg: 'bg-yellow-200', border: 'border-yellow-300' }, '0': { bg: 'bg-green-200', border: 'border-green-300' }, '1': { bg: 'bg-yellow-200', border: 'border-yellow-300' }, '2': { bg: 'bg-red-200', border: 'border-red-300' }, } const RANGE_COLOR_MAP = Object.fromEntries( Object.entries(COLOR_VALUES).map(([key, value]) => [key, value.bg]) ) const BORDER_COLOR_MAP = Object.fromEntries( Object.entries(COLOR_VALUES).map(([key, value]) => [key, value.border]) )This approach centralizes the color values and makes it easier to maintain or update them in the future.
149-149: AwaitlocalForage.setItemto ensure data persistenceThe
localForage.setItemoperation is asynchronous and should be awaited to guarantee that the data is saved before proceeding. This is crucial, especially if subsequent code relies on this data being stored.Modify the code as follows:
- localForage.setItem(`${quizId}-confusion`, { + await localForage.setItem(`${quizId}-confusion`, { prevSpeed: speed, prevDifficulty: difficulty, prevTimestamp: dayjs().format(), })packages/graphql/src/services/liveQuizzes.ts (1)
630-658: Optimize feedback aggregation using efficient filteringIn the
aggregateFeedbacksfunction, filtering feedbacks withArray.filteranddayjs().diffcan be inefficient for larger datasets. Consider sorting the feedbacks bycreatedAtin descending order and breaking the loop once feedbacks are outside the desired time window to improve performance.Apply this diff to optimize the aggregation:
const aggregateFeedbacks = (feedbacks: ConfusionTimestep[]) => { - // TODO: for improved efficiency, try to use descending feedback ordering - // and break early once first is not within the filtering requirements anymore - const recentFeedbacks = feedbacks.filter( - (feedback) => - dayjs().diff(dayjs(feedback.createdAt)) > 0 && - dayjs().diff(dayjs(feedback.createdAt)) < 1000 * 60 * 10 - ) + const tenMinutesInMillis = 1000 * 60 * 10 + const now = dayjs() + const recentFeedbacks = [] + for (const feedback of feedbacks.sort((a, b) => b.createdAt.getTime() - a.createdAt.getTime())) { + const diff = now.diff(dayjs(feedback.createdAt)) + if (diff >= 0 && diff <= tenMinutesInMillis) { + recentFeedbacks.push(feedback) + } else if (diff > tenMinutesInMillis) { + break + } + } if (recentFeedbacks.length > 0) { const summedFeedbacks = recentFeedbacks.reduce( (previousValue, feedback) => { return { speed: previousValue.speed + feedback.speed, difficulty: previousValue.difficulty + feedback.difficulty, numberOfParticipants: previousValue.numberOfParticipants + 1, } }, { speed: 0, difficulty: 0, numberOfParticipants: 0 } ) return { ...summedFeedbacks, speed: summedFeedbacks.speed / summedFeedbacks.numberOfParticipants, difficulty: summedFeedbacks.difficulty / summedFeedbacks.numberOfParticipants, } } return { speed: 0, difficulty: 0, numberOfParticipants: 0 } }
🛑 Comments failed to post (19)
packages/graphql/src/graphql/ops/MCreateFeedback.graphql (1)
1-2: 💡 Codebase verification
Inconsistent parameter naming found across GraphQL operations
The transition from
sessionIdtoquizIdis not consistently applied across the codebase. Several files still usesessionId:
- Session block operations:
MActivateSessionBlock.graphqlMDeactivateSessionBlock.graphql- Session queries:
QGetSessionLeaderboard.graphqlQGetSessionEvaluation.graphql(uses bothsessionIdandid)- The GraphQL schema (
schema.graphql) still defines operations withsessionIdparametersThis indicates that the architecture transition from session-based to quiz-based logic is incomplete. The parameter renaming in
MCreateFeedback.graphqlappears to be part of an ongoing migration that needs to be completed across all related operations.🔗 Analysis chain
LGTM! Parameter renaming aligns with architecture changes.
The transition from
sessionIdtoquizIdis consistent with the codebase-wide refactoring from session-based to quiz-based logic.Let's verify that this schema change is consistently applied across related GraphQL operations:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify consistent parameter naming across GraphQL operations # Expected: No remaining instances of sessionId in GraphQL operations # Test 1: Check for any remaining sessionId parameters in mutations echo "Checking mutations for sessionId..." fd -e graphql -x rg -l 'mutation.*\$sessionId:' {} # Test 2: Check for any remaining sessionId parameters in queries echo "Checking queries for sessionId..." fd -e graphql -x rg -l 'query.*\$sessionId:' {} # Test 3: Check for any remaining sessionId field usage echo "Checking field usage for sessionId..." fd -e graphql -x rg -l '\(sessionId:' {}Length of output: 833
Script:
#!/bin/bash # Let's examine the content of these files to understand the context better echo "=== Content of MDeactivateSessionBlock.graphql ===" cat ./packages/graphql/src/graphql/ops/MDeactivateSessionBlock.graphql echo -e "\n=== Content of MActivateSessionBlock.graphql ===" cat ./packages/graphql/src/graphql/ops/MActivateSessionBlock.graphql echo -e "\n=== Content of QGetSessionLeaderboard.graphql ===" cat ./packages/graphql/src/graphql/ops/QGetSessionLeaderboard.graphql echo -e "\n=== Content of QGetSessionEvaluation.graphql ===" cat ./packages/graphql/src/graphql/ops/QGetSessionEvaluation.graphql echo -e "\n=== Relevant parts of schema.graphql containing sessionId ===" rg "sessionId" ./packages/graphql/src/public/schema.graphql -C 2Length of output: 4927
packages/graphql/src/graphql/ops/SFeedbackAdded.graphql (1)
1-2: 💡 Codebase verification
Mixed usage of
sessionIdandquizIdfound in the codebaseThe verification reveals inconsistent parameter naming across GraphQL operations:
Several operations still use
sessionId:
activateSessionBlock,deactivateSessionBlockmutationssessionLeaderboardquery- Associated operation files (QGetSessionLeaderboard, QGetSessionEvaluation, etc.)
While feedback-related operations have been updated to use
quizId:
- All feedback subscriptions (Added/Created/Updated/Removed)
- Feedback queries and mutations
- Live quiz related operations
This mixed usage suggests an incomplete refactoring effort that could lead to runtime issues. The schema should consistently use either session-based or quiz-based terminology.
🔗 Analysis chain
LGTM! Verify schema consistency across the codebase.
The parameter and field renaming from
sessionIdtoquizIdaligns well with the refactoring objectives. The subscription structure remains robust with comprehensive fields for feedback handling.Let's verify the schema changes are consistent across the codebase:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that all sessionId references in GraphQL files have been updated to quizId # and there are no mixed usages that could cause runtime issues. echo "Checking for any remaining sessionId references in GraphQL files..." rg -g '*.graphql' -g '*.gql' 'sessionId' echo "Checking for quizId usage consistency in GraphQL files..." rg -g '*.graphql' -g '*.gql' 'quizId' echo "Checking GraphQL schema for type definitions..." fd -g 'schema.graphql' --exec cat {} \; | rg -A 5 -B 5 'type .*Feedback|input .*Feedback'Length of output: 5968
packages/graphql/src/graphql/ops/SFeedbackUpdated.graphql (1)
1-2: 💡 Codebase verification
Warning: Inconsistent parameter naming across related operations
The verification revealed that while feedback-related operations consistently use
quizId, there are still several session-related operations usingsessionId. This indicates an incomplete transition:
QGetSessionLeaderboard.graphql: UsessessionIdparameterQGetSessionEvaluation.graphql: UsessessionIdparameterMDeactivateSessionBlock.graphql: UsessessionIdparameterMActivateSessionBlock.graphql: UsessessionIdparameterThe parameter renaming in
SFeedbackUpdated.graphqlis part of a larger architectural change, but the codebase shows inconsistent implementation of this change.🔗 Analysis chain
LGTM! Parameter renaming aligns with the quiz-based architecture.
The change from
sessionIdtoquizIdis consistent with the broader refactoring effort.Let's verify that this renaming is consistently applied across all related GraphQL operations:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for any remaining sessionId references in GraphQL files # and verify the consistency of quizId usage # Search for any remaining sessionId references in GraphQL files echo "Checking for remaining sessionId references in GraphQL operations:" rg -t graphql 'sessionId' packages/graphql/src/graphql/ops/ # Verify consistent usage of quizId in feedback-related operations echo "Verifying quizId usage in feedback-related operations:" rg -t graphql 'quizId.*feedback' packages/graphql/src/graphql/ops/Length of output: 454
Script:
#!/bin/bash # Let's try without file type filtering since GraphQL extension isn't recognized echo "Checking for remaining sessionId references:" rg "sessionId" packages/graphql/src/graphql/ops/ echo -e "\nVerifying quizId usage in feedback operations:" rg "quizId.*feedback|feedback.*quizId" packages/graphql/src/graphql/ops/ echo -e "\nListing all GraphQL operation files for context:" find packages/graphql/src/graphql/ops/ -type f -name "*.graphql"Length of output: 13528
apps/frontend-pwa/src/components/liveSession/FeedbackAreaSubscriber.tsx (2)
20-35: 🛠️ Refactor suggestion
Consider adding error handling and timestamp-based sorting.
The current implementation has two potential issues:
- No error handling for subscription setup failures
- Multiple rapid feedback additions might lead to inconsistent ordering
Consider this improvement:
const feedbackAdded = subscribeToMore({ document: FeedbackAddedDocument, variables: { quizId }, + onError: (error) => { + console.error('Error subscribing to feedback updates:', error) + }, updateQuery: ( prev: { feedbacks: Feedback[] }, { subscriptionData, }: { subscriptionData: { data: { feedbackAdded: Feedback } } } ) => { if (!subscriptionData.data) return prev const newItem = subscriptionData.data.feedbackAdded if (prev.feedbacks?.map((item) => item.id).includes(newItem.id)) return prev - return { ...prev, feedbacks: [newItem, ...prev.feedbacks] } + const updatedFeedbacks = [newItem, ...prev.feedbacks] + // Sort by creation timestamp to ensure consistent ordering + updatedFeedbacks.sort((a, b) => b.createdAt - a.createdAt) + return { ...prev, feedbacks: updatedFeedbacks } }, })Committable suggestion skipped: line range outside the PR's diff.
37-55: 🛠️ Refactor suggestion
Improve type safety in ID handling.
The current implementation mixes string and number types for IDs, which could lead to subtle bugs.
Consider using consistent ID types:
updateQuery: ( prev: { feedbacks: Feedback[] }, { subscriptionData, }: { subscriptionData: { data: { feedbackRemoved: string } } } ) => { if (!subscriptionData.data) return prev const removedItem = subscriptionData.data.feedbackRemoved return { ...prev, feedbacks: prev.feedbacks?.filter( - (item) => item.id !== parseInt(removedItem) + (item) => item.id.toString() !== removedItem ), } },📝 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 feedbackRemoved = subscribeToMore({ document: FeedbackRemovedDocument, variables: { quizId }, updateQuery: ( prev: { feedbacks: Feedback[] }, { subscriptionData, }: { subscriptionData: { data: { feedbackRemoved: string } } } ) => { if (!subscriptionData.data) return prev const removedItem = subscriptionData.data.feedbackRemoved return { ...prev, feedbacks: prev.feedbacks?.filter( (item) => item.id.toString() !== removedItem ), } }, })apps/frontend-pwa/src/components/liveSession/PublicFeedback.tsx (2)
80-80:
⚠️ Potential issueAdd error handling and loading states for async operations.
The async operations lack error handling and user feedback. Consider implementing:
- Try-catch blocks for async operations
- Loading states during operations
- User feedback for success/failure
Example implementation:
const onUpvote = async (previousValue: boolean) => { + try { + setIsLoading(true); const newUpvotes = { ...upvotes, upvote: !previousValue } setUpvotes(newUpvotes) await localForage.setItem( `${feedbackId}-upvotes`, JSON.stringify(newUpvotes) ) await onUpvoteFeedback(feedbackId, previousValue ? -1 : 1) + // Optional: Show success toast/feedback + } catch (error) { + console.error('Failed to update upvote:', error); + setUpvotes({ ...upvotes }); // Rollback state + // Show error toast/feedback + } finally { + setIsLoading(false); + } }Also applies to: 99-103, 123-127
170-171: 🛠️ Refactor suggestion
Prevent rapid clicks and show loading state.
The click handlers could lead to race conditions with rapid clicks. Consider:
- Debouncing/throttling click events
- Disabling buttons during operations
- Adding loading indicators
Example implementation:
+ const [isProcessing, setIsProcessing] = useState<Record<number, boolean>>({}); <Button onClick={async () => { + if (isProcessing[response.id]) return; + setIsProcessing(prev => ({ ...prev, [response.id]: true })); try { await onResponseUpvote(upvotes[response.id], response.id); + } finally { + setIsProcessing(prev => ({ ...prev, [response.id]: false })); + } }} + disabled={isProcessing[response.id]} active={upvotes[response.id] === 1} className={{ root: 'mr-1 h-9 w-9' }} > <Button.Icon> + {isProcessing[response.id] ? <Spinner size="sm" /> : ( <FontAwesomeIcon icon={faThumbsUp} size="lg" /> + )} </Button.Icon> </Button>Also applies to: 184-188
packages/graphql/src/schema/course.ts (1)
98-106: 💡 Codebase verification
Consider tracking migration TODOs centrally
The search results confirm multiple TODO comments related to the "live quiz migration" scattered across the codebase, particularly in:
- packages/types/src/index.ts (6 instances)
- packages/graphql/src/schema/ (multiple files)
- packages/graphql/src/services/
Recommendations:
- Create a central tracking issue for the live quiz migration
- Update all related TODOs to reference the issue number (e.g.,
// TODO(#123): remove after migration)- Consider using a consistent format to make future cleanup easier
🔗 Analysis chain
Consider consolidating duplicate TODO comments.
The implementation of both fields is correct, but there are duplicate TODO comments about session removal. Consider:
- Centralizing the TODO tracking in a single location
- Using a more specific comment format that can be tracked by tools (e.g.,
// TODO(#issue): remove after migration)Let's check for other similar TODOs in the codebase:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for similar TODO comments about session migration rg -i "todo.*remove.*migration" --type tsLength of output: 266
Script:
#!/bin/bash # Let's try a more specific search for TODO comments about sessions rg -i "todo.*session" --type ts # Also search for similar migration-related TODOs rg -i "todo.*migrat" --type ts # Let's also check the specific file for context cat "packages/graphql/src/schema/course.ts"Length of output: 11599
apps/frontend-pwa/src/pages/session/[id].tsx (1)
31-52:
⚠️ Potential issueFix inconsistencies in the updateQuery function.
There are two issues in the updateQuery function:
- The returned object uses
sessionproperty but the previous state usesstudentLiveQuiz- The spread operation is using
prev.studentLiveQuizbut trying to assign it to asessionpropertyApply this diff to fix the property names:
return Object.assign({}, prev, { - session: { - ...prev.studentLiveQuiz, + studentLiveQuiz: { + ...prev.studentLiveQuiz, activeBlock: subscriptionData.data.runningLiveQuizUpdated, }, })📝 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.id: string subscribeToMore: (doc: SubscribeToMoreOptions) => any }) { useEffect(() => { subscribeToMore({ document: RunningLiveQuizUpdatedDocument, variables: { quizId: id, }, updateQuery: ( prev: { studentLiveQuiz: LiveQuiz }, { subscriptionData, }: { subscriptionData: { data: { runningLiveQuizUpdated: ElementBlock } } } ) => { if (!subscriptionData.data) return prev return Object.assign({}, prev, { studentLiveQuiz: { ...prev.studentLiveQuiz, activeBlock: subscriptionData.data.runningLiveQuizUpdated,packages/i18n/messages/en.ts (1)
128-128: 💡 Codebase verification
Several occurrences of "session" still need to be updated to "quiz" for consistency
The search results reveal several instances where "session" terminology is still being used in both English and German translations. Here are the key areas that need attention:
In
packages/i18n/messages/en.ts:
sessionobject key and its nested translationsSESSION_EXECconstantsessionName,sessionDisplayName,sessionControl, etc.- References to "session" in descriptive texts about Q&A and feedback channels
In
packages/i18n/messages/de.ts:
- Similar occurrences of "Session" in German translations that should be updated to "Quiz"
SESSION_EXECand related terminology- Various UI elements and messages still using "Session" instead of "Quiz"
🔗 Analysis chain
LGTM! Verify other occurrences of "session" in translations.
The renaming from
activeSessionstoactiveLiveQuizzesaligns with the PR objective of transitioning from session-based to quiz-based logic.Let's verify if there are any remaining inconsistent usages of "session" that should be updated to "quiz":
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Search for remaining occurrences of "session" in translation files # that might need to be updated for consistency. # Test: Search for "session" in translation files rg -i "session" "packages/i18n/messages/"Length of output: 10374
apps/frontend-pwa/src/lib/hooks/useStudentOverviewSplit.tsx (2)
129-131: 🛠️ Refactor suggestion
Ensure 'isCompleted' always returns a boolean value.
The expression
participation.completedMicroLearnings?.includes(micro.id)may returnundefinedifcompletedMicroLearningsis undefined. This could result inisCompletedbeingundefinedinstead of a boolean. To ensureisCompletedis always a boolean, provide a default value.Apply this diff to guarantee a boolean value:
isCompleted: participation.completedMicroLearnings?.includes( micro.id - ), + ) ?? false,This change ensures that if
includesreturnsundefined,isCompletedwill default tofalse.📝 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.isCompleted: participation.completedMicroLearnings?.includes( micro.id ) ?? false,
77-79: 🛠️ Refactor suggestion
Correct the comment to reflect the actual date comparison logic.
The comment on line 77 states:
// check if endDate of course is before today or today, but the condition checks if theendDateis after or same as today usingisAfter(dayjs()) || isSame(dayjs()). The comment should be updated to accurately describe the logic.Apply this diff to correct the comment:
-// check if endDate of course is before today or today +// Check if the endDate of the course is today or in the future📝 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.// Check if the endDate of the course is today or in the future dayjs(participation.course?.endDate).isAfter(dayjs()) || dayjs(participation.course?.endDate).isSame(dayjs())packages/graphql/src/services/feedbacks.ts (3)
111-111:
⚠️ Potential issueAvoid Using Non-Null Assertion '!' on Potentially Undefined Properties
Using
feedback.liveQuiz!assumes thatliveQuizis notnullorundefined, which could lead to runtime errors if it is. Implement proper null checks to ensure robustness.Modify the condition to include a null check:
-if (!feedback || feedback.liveQuiz!.ownerId !== ctx.user.sub) return null +if (!feedback || !feedback.liveQuiz || feedback.liveQuiz.ownerId !== ctx.user.sub) 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.if (!feedback || !feedback.liveQuiz || feedback.liveQuiz.ownerId !== ctx.user.sub) return null
231-231:
⚠️ Potential issueEnsure Proper Null Checks Before Accessing 'ownerId' in 'pinFeedback'
As with previous instances, avoid using non-null assertions on
liveQuiz. Implement null checks to prevent potential runtime errors.Update the condition:
-if (!feedback || feedback.liveQuiz!.ownerId !== ctx.user.sub) return null +if (!feedback || !feedback.liveQuiz || feedback.liveQuiz.ownerId !== ctx.user.sub) 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.if (!feedback || !feedback.liveQuiz || feedback.liveQuiz.ownerId !== ctx.user.sub) return null
333-333:
⚠️ Potential issueAvoid Non-Null Assertion in 'deleteFeedbackResponse' and Check for Null
Ensure that
feedbackResponse.feedback.liveQuizis notnullbefore accessingownerIdto prevent runtime errors.Update the condition:
-if (!feedbackResponse || feedbackResponse.feedback.liveQuiz!.ownerId !== ctx.user.sub) { +if ( + !feedbackResponse || + !feedbackResponse.feedback.liveQuiz || + feedbackResponse.feedback.liveQuiz.ownerId !== ctx.user.sub +) {Committable suggestion skipped: line range outside the PR's diff.
apps/frontend-pwa/src/components/liveSession/FeedbackArea.tsx (2)
77-84: 🛠️ Refactor suggestion
Add error handling for asynchronous mutations in
onAddFeedbackThe
onAddFeedbackfunction performs an asynchronous mutation without error handling. If the mutation fails, the user won't be notified, and potential errors will go unhandled. Consider adding a try-catch block to handle errors and provide user feedback.You can modify the function as follows:
const onAddFeedback = async (input: string) => { if (!router.query.id) return + try { await createFeedback({ variables: { quizId: router.query.id as string, content: input, }, }) + } catch (error) { + // Handle error, e.g., show notification to user + console.error('Failed to add feedback:', error) + } }Committable suggestion skipped: line range outside the PR's diff.
143-149:
⚠️ Potential issueAwait asynchronous operations and handle errors in
handleNewConfusionTSIn the
handleNewConfusionTSfunction, theaddConfusionTimestepmutation andlocalForage.setItemare both asynchronous operations but are not being awaited. This may lead to unhandled promise rejections and unexpected behavior. To ensure proper execution and error handling, you shouldawaitthese operations and include error handling.Modify the code as follows:
- addConfusionTimestep({ + await addConfusionTimestep({ variables: { quizId, difficulty: difficulty, speed: speed, }, })- localForage.setItem(`${quizId}-confusion`, { + await localForage.setItem(`${quizId}-confusion`, { prevSpeed: speed, prevDifficulty: difficulty, prevTimestamp: dayjs().format(), })Also, ensure that these await statements are within the existing try-catch block to handle any potential errors effectively.
Committable suggestion skipped: line range outside the PR's diff.
packages/graphql/src/services/liveQuizzes.ts (2)
518-547:
⚠️ Potential issueAdd error handling when the live quiz is not found
In the
changeLiveQuizSettingsfunction, if the live quiz with the specifiedidandownerIddoes not exist,prisma.liveQuiz.updatewill throw an error. It's recommended to check whether the quiz exists before attempting to update it to provide a clearer error message.Apply this diff to add error handling:
export async function changeLiveQuizSettings( { id, isLiveQAEnabled, isConfusionFeedbackEnabled, isModerationEnabled, isGamificationEnabled, }: { id: string isLiveQAEnabled?: boolean | null isConfusionFeedbackEnabled?: boolean | null isModerationEnabled?: boolean | null isGamificationEnabled?: boolean | null }, ctx: ContextWithUser ) { + const existingQuiz = await ctx.prisma.liveQuiz.findFirst({ + where: { + id, + ownerId: ctx.user.sub, + }, + }) + if (!existingQuiz) { + throw new GraphQLError('Live quiz not found or access denied') + } const quiz = await ctx.prisma.liveQuiz.update({ where: { id, ownerId: ctx.user.sub, }, data: { isLiveQAEnabled: isLiveQAEnabled ?? undefined, isConfusionFeedbackEnabled: isConfusionFeedbackEnabled ?? undefined, isModerationEnabled: isModerationEnabled ?? undefined, isGamificationEnabled: isGamificationEnabled ?? undefined, }, }) return quiz }📝 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.export async function changeLiveQuizSettings( { id, isLiveQAEnabled, isConfusionFeedbackEnabled, isModerationEnabled, isGamificationEnabled, }: { id: string isLiveQAEnabled?: boolean | null isConfusionFeedbackEnabled?: boolean | null isModerationEnabled?: boolean | null isGamificationEnabled?: boolean | null }, ctx: ContextWithUser ) { const existingQuiz = await ctx.prisma.liveQuiz.findFirst({ where: { id, ownerId: ctx.user.sub, }, }) if (!existingQuiz) { throw new GraphQLError('Live quiz not found or access denied') } const quiz = await ctx.prisma.liveQuiz.update({ where: { id, ownerId: ctx.user.sub, }, data: { isLiveQAEnabled: isLiveQAEnabled ?? undefined, isConfusionFeedbackEnabled: isConfusionFeedbackEnabled ?? undefined, isModerationEnabled: isModerationEnabled ?? undefined, isGamificationEnabled: isGamificationEnabled ?? undefined, }, }) return quiz }
552-608: 🛠️ Refactor suggestion
Ensure consistent handling of deletion results
In the
deleteLiveQuizfunction, the return value isnullin cases where the quiz cannot be deleted. For better clarity and consistency, consider throwing aGraphQLErrorwhen deletion is not possible due to the quiz's status or non-existence, so that the client can handle the error appropriately.Apply this diff to improve error handling:
if (!liveQuiz) { - return null + throw new GraphQLError('Live quiz not found or access denied') } if (liveQuiz.status === PublicationStatus.PUBLISHED) { // running live quizzes cannot be deleted - return null + throw new GraphQLError('Cannot delete a running live quiz') } else if (liveQuiz.status === PublicationStatus.ENDED) { // ... existing code ... } else { // ... existing code ... }📝 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.export async function deleteLiveQuiz( { id }: { id: string }, ctx: ContextWithUser ) { // fetch live quiz to check its status const liveQuiz = await ctx.prisma.liveQuiz.findUnique({ where: { id, ownerId: ctx.user.sub, }, select: { status: true, }, }) if (!liveQuiz) { throw new GraphQLError('Live quiz not found or access denied') } if (liveQuiz.status === PublicationStatus.PUBLISHED) { // running live quizzes cannot be deleted throw new GraphQLError('Cannot delete a running live quiz') } else if (liveQuiz.status === PublicationStatus.ENDED) { const deletedLiveQuiz = await ctx.prisma.liveQuiz.update({ where: { id, ownerId: ctx.user.sub, status: PublicationStatus.ENDED, }, data: { isDeleted: true, }, }) ctx.emitter.emit('invalidate', { typename: 'Session', id, }) return deletedLiveQuiz } else { const deletedLiveQuiz = await ctx.prisma.liveQuiz.delete({ where: { id, ownerId: ctx.user.sub, status: { in: [PublicationStatus.DRAFT, PublicationStatus.SCHEDULED], }, }, }) ctx.emitter.emit('invalidate', { typename: 'LiveQuiz', id, }) return deletedLiveQuiz } }
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: 9
🧹 Outside diff range and nitpick comments (27)
packages/graphql/src/graphql/ops/MCancelLiveQuiz.graphql (1)
1-5: Consider enhancing the response type with status information.While returning just the ID is sufficient for basic confirmation, consider adding fields like
successorstatusto provide more detailed feedback about the cancellation operation. This could help in better error handling on the client side.mutation CancelLiveQuiz($id: String!) { cancelLiveQuiz(id: $id) { id + success + message } }packages/graphql/src/graphql/ops/QGetCourseRunningLiveQuizzes.graphql (1)
1-11: Consider adding pagination and additional metadata fields.For improved scalability and usability, consider:
- Adding pagination support for courses with many running quizzes
- Including additional metadata fields like:
- Quiz start time
- Current status
- Number of participants
query GetCourseRunningLiveQuizzes($courseId: String!) { - getCourseRunningLiveQuizzes(courseId: $courseId) { + getCourseRunningLiveQuizzes( + courseId: $courseId + first: $first + after: $cursor + ) { + edges { + node { id name displayName + startedAt + status + participantCount course { id displayName } + } + } + pageInfo { + hasNextPage + endCursor + } } }apps/frontend-manage/src/components/courses/LiveQuizList.tsx (1)
1-1: Consider renaming SessionStatus and related constants to align with quiz-based terminology.The codebase is transitioning from sessions to quizzes, but
SessionStatusandsortingOrderSessionsstill use session-based terminology. This could lead to confusion.Consider applying these changes:
-import { SessionStatus } from '@klicker-uzh/graphql/dist/ops' +import { QuizStatus } from '@klicker-uzh/graphql/dist/ops' -const sortingOrderSessions: Record<string, number> = { - [SessionStatus.Running]: 0, - [SessionStatus.Scheduled]: 1, - [SessionStatus.Prepared]: 2, - [SessionStatus.Completed]: 3, +const sortingOrderQuizzes: Record<string, number> = { + [QuizStatus.Running]: 0, + [QuizStatus.Scheduled]: 1, + [QuizStatus.Prepared]: 2, + [QuizStatus.Completed]: 3, }Also applies to: 6-11
packages/graphql/src/graphql/ops/QGetSingleCourse.graphql (1)
30-32: LGTM! Consider documenting the feature flags.The new boolean fields for controlling live quiz features are well-named and align with the PR objectives.
Consider adding GraphQL descriptions to document the purpose and impact of each feature flag. Example:
+ """ + Controls whether live Q&A functionality is enabled for this quiz + """ isLiveQAEnabled + """ + Controls whether participants can provide confusion feedback + """ isConfusionFeedbackEnabled + """ + Controls whether content moderation is enabled for this quiz + """ isModerationEnabledapps/frontend-manage/src/components/courses/actions/StartLiveQuizButton.tsx (3)
Line range hint
24-36: Simplify cache update logic and strengthen type safety.The current implementation has redundant null checks and could be simplified:
- The type
Pick<LiveQuiz, 'id' | 'name'>guarantees that both fields exist- The null check condition might allow empty strings
- The null coalescing for empty array is repeated
Consider this more robust implementation:
cache.writeQuery({ query: GetUserRunningLiveQuizzesDocument, data: { - userRunningLiveQuizzes: - liveQuiz.id && liveQuiz.name - ? [ - ...(data?.userRunningLiveQuizzes ?? []), - { id: liveQuiz.id, name: liveQuiz.name }, - ] - : (data?.userRunningLiveQuizzes ?? []), + userRunningLiveQuizzes: [ + ...(data?.userRunningLiveQuizzes ?? []), + { id: liveQuiz.id, name: liveQuiz.name }, + ], }, })
Line range hint
44-49: Remove unnecessary fallback and improve error handling.Several improvements can be made to the click handler:
- The
|| ''fallback is unnecessary asidis guaranteed by the type- Error handling should provide feedback to the user
Consider this improved implementation:
onClick={async () => { try { await startLiveQuiz({ - variables: { id: liveQuiz.id || '' }, + variables: { id: liveQuiz.id }, }) router.push(`/sessions/${liveQuiz.id}/cockpit`) } catch (error) { - console.log(error) + // Assuming you have a toast notification system + showErrorToast(t('manage.sessions.startError')) + console.error('Failed to start live quiz:', error) } }}
Line range hint
41-65: Add loading state feedback for better UX.While
startingQuizis tracked, there's no visual feedback to users during the loading state.Consider adding loading indicators:
<Button basic disabled={startingQuiz} + loading={startingQuiz} onClick={async () => { // ... existing code ... }} className={{ root: 'text-primary-100' }} data={{ cy: `start-live-quiz-${liveQuiz.name}` }} > <Button.Icon> - <FontAwesomeIcon icon={faPlay} /> + <FontAwesomeIcon icon={faPlay} className={startingQuiz ? 'opacity-50' : ''} /> </Button.Icon> - <Button.Label>{t('manage.sessions.startLiveQuiz')}</Button.Label> + <Button.Label> + {startingQuiz ? t('manage.sessions.starting') : t('manage.sessions.startLiveQuiz')} + </Button.Label> </Button>apps/frontend-manage/src/components/sessions/cockpit/SessionAbortionConfirmations.tsx (1)
Rename component and related types to align with quiz-based terminology
The verification confirms the need for renaming to maintain consistency with quiz-based terminology. The changes are localized to two files with clear dependencies:
In
SessionAbortionConfirmations.tsx:
- Rename component to
LiveQuizAbortionConfirmations- Rename props interface to
LiveQuizAbortionConfirmationsProps- Rename file to
LiveQuizAbortionConfirmations.tsxIn
CancelLiveQuizModal.tsx:
- Rename type to
LiveQuizAbortionConfirmationType- Update import statement
- Update component usage
This change aligns with the modal's existing naming pattern (CancelLiveQuizModal) and the GraphQL operations already using "LiveQuiz" terminology (GetLiveQuizSummary, CancelLiveQuiz).
🔗 Analysis chain
Line range hint
1-101: Consider renaming component for terminology consistency.For consistency with the new quiz-based terminology, consider renaming:
- Component from
SessionAbortionConfirmationstoLiveQuizAbortionConfirmations- Type from
SessionAbortionConfirmationTypetoLiveQuizAbortionConfirmationType- File from
SessionAbortionConfirmations.tsxtoLiveQuizAbortionConfirmations.tsxLet's verify the scope of this rename:
Let me check the CancelLiveQuizModal component to understand the complete context.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for other occurrences of SessionAbortion to assess the impact # of the suggested rename echo "Checking for SessionAbortion references:" rg "SessionAbortion"Length of output: 1742
Script:
#!/bin/bash # Check the content of CancelLiveQuizModal to understand the component's context cat apps/frontend-manage/src/components/sessions/cockpit/CancelLiveQuizModal.tsxLength of output: 4342
apps/frontend-manage/src/components/sessions/cockpit/CancelLiveQuizModal.tsx (3)
Line range hint
1-20: Consider updating file path and type names for consistency.The codebase is transitioning from session-based to quiz-based terminology, but some naming inconsistencies remain:
- The file is still located in the
sessions/cockpitdirectorySessionAbortionConfirmationTypeinterface still uses session-based terminologyConsider:
- Moving the file to a more appropriate directory (e.g.,
quizzes/cockpit)- Renaming the interface to
QuizAbortionConfirmationType
Line range hint
57-76: Update loading state variable name for consistency.The loading state variable
sessionDeletingshould be renamed toquizDeletingto maintain consistency with the quiz-based terminology.- const [cancelLiveQuiz, { loading: sessionDeleting }] = useMutation( + const [cancelLiveQuiz, { loading: quizDeleting }] = useMutation(
Line range hint
124-135: Update data attributes and navigation path for consistency.Several UI elements still use session-based terminology:
- The navigation path is still '/sessions'
- The data-cy attributes use 'session' in their names
- router.push('/sessions') + router.push('/quizzes') setOpen(false) setConfirmations({ ...initialConfirmations }) }} className={{ root: 'bg-red-700 text-white hover:bg-red-800 hover:text-white disabled:bg-opacity-50 disabled:hover:cursor-not-allowed', }} - data={{ cy: 'confirm-cancel-session' }} + data={{ cy: 'confirm-cancel-quiz' }}apps/frontend-pwa/src/pages/course/[courseId]/liveQuizzes.tsx (2)
35-46: Consider enhancing the loading state with a descriptive message.While the loading state implementation is good, it could be more informative for users.
Consider updating the loader with a descriptive message:
if (loading) { return ( <Layout> - <Loader /> + <div className="flex flex-col items-center gap-2"> + <Loader /> + <div className="text-sm text-gray-600"> + {t('pwa.general.loadingLiveQuizzes')} + </div> + </div> </Layout> ) }
Line range hint
123-129: Enhance error handling in getServerSideProps.The current check might not catch all edge cases.
Consider more specific error handling:
- if (!result?.data?.getCourseRunningLiveQuizzes) { + if (!result?.data?.getCourseRunningLiveQuizzes || result.errors) { return { props: { isInactive: true, + error: result.errors?.[0]?.message ?? 'Failed to fetch live quizzes', }, } }apps/frontend-manage/src/components/courses/CourseOverviewHeader.tsx (1)
23-26: Consider adding JSDoc documentation for better type clarity.The interface would benefit from JSDoc documentation explaining the purpose of each omitted field and why they're excluded.
+/** + * Props for the CourseOverviewHeader component + * @property {Course} course - Course object excluding leaderboard, liveQuizzes, + * practiceQuizzes, and microLearnings fields + */ interface CourseOverviewHeaderProps { course: Omit< Course, 'leaderboard' | 'liveQuizzes' | 'practiceQuizzes' | 'microLearnings' >apps/frontend-manage/src/components/sessions/cockpit/SessionTimeline.tsx (3)
320-324: Consider renaming sessionId prop for consistency.While the modal component has been updated to reflect quiz-based terminology, the
sessionIdprop name remains unchanged. Consider renaming it toquizIdfor consistency with the broader refactoring effort.<CancelLiveQuizModal open={cancelLiveQuizModal} setOpen={setCancelLiveQuizModal} - sessionId={sessionId} + quizId={sessionId} title={sessionName} />
Line range hint
35-46: Update interface name and props for consistency.The interface still uses session-based terminology while the codebase is transitioning to quiz-based naming. Consider updating the interface for consistency.
-interface SessionTimelineProps { +interface QuizTimelineProps { blocks?: QuizTimelineBlock[] - sessionName: string + quizName: string handleEndSession: () => void handleTogglePublicEvaluation: () => void handleOpenBlock: (blockId: number) => void handleCloseBlock: (blockId: number) => void isEvaluationPublic?: boolean - sessionId: string + quizId: string startedAt?: string loading?: boolean }This change would require updating the component's parameter destructuring and usage accordingly.
Line range hint
48-48: Rename component to QuizTimeline.For consistency with the quiz-based terminology transition, consider renaming the component.
-function SessionTimeline({ +function QuizTimeline({ sessionId, blocks = [], sessionName, // ... other props }: SessionTimelineProps): React.ReactElement { // ... component implementation } -export default SessionTimeline +export default QuizTimelineAlso applies to: 334-334
packages/graphql/src/services/liveQuizzes.ts (4)
Line range hint
26-207: Consider splitting themanipulateLiveQuizfunction for better maintainability.The function handles both creation and updates with complex logic. Consider splitting it into smaller, focused functions:
validateQuizElementsprocessQuizBlockscreateOrUpdateQuizThis would improve readability and make the code easier to maintain.
Line range hint
322-518: Consider security improvements in quiz execution.
- The pin code generation uses
Math.random()which isn't cryptographically secure. Consider usingcrypto.randomInt()instead.- Redis keys should be prefixed with a version to prevent conflicts during deployments.
Apply this diff for secure pin code generation:
- const pinCode = 100000 + Math.floor(Math.random() * 900000) + const pinCode = 100000 + crypto.randomInt(900000)
809-811: Address the TODO comment for feedback aggregation performance.The comment suggests using descending order for early breaking. This would indeed improve performance.
Would you like me to help implement this optimization or create a GitHub issue to track this improvement?
Line range hint
840-935: Improve type safety in student getter functions.Replace
anytype with proper interfaces:- let quizWithoutSolutions: any + interface QuizWithoutSolutions { + id: string + activeBlock?: { + elements: Array<{ + elementData: ElementData + // ... other properties + }> + // ... other properties + } + // ... other properties + } + let quizWithoutSolutions: QuizWithoutSolutionspackages/graphql/src/services/sessions.ts (3)
Line range hint
45-303: Consider improving transaction safety in endSession functionThe function handles multiple database operations, but not all of them are included in the transaction. This could lead to inconsistent state if some operations fail.
Consider wrapping all database operations (including the final session update) in a single transaction:
- await ctx.prisma.$transaction(promises) - - const keys = await ctx.redisExec.keys(`s:${id}:*`) - const pipe = ctx.redisExec.multi() - for (const key of keys) { - pipe.unlink(key) - } - await pipe.exec() - - const stoppedSession = await ctx.prisma.liveSession.update({ + const stoppedSession = await ctx.prisma.$transaction(async (tx) => { + await Promise.all(promises) + + const keys = await ctx.redisExec.keys(`s:${id}:*`) + const pipe = ctx.redisExec.multi() + for (const key of keys) { + pipe.unlink(key) + } + await pipe.exec() + + return tx.liveSession.update({ where: { id, }, data: { status: SessionStatus.COMPLETED, finishedAt: new Date(), pinCode: null, }, + }) })
Line range hint
892-939: Optimize statistics computation performanceThe
computeStatisticsfunction performs expensive calculations that could be optimized:
- Results could be memoized to prevent recalculation
- Edge cases could be handled more efficiently
Consider implementing memoization:
+ const memoizedResults = new Map() + function computeStatistics(instance: PickedInstanceType) { + const cacheKey = JSON.stringify(instance.results) + if (memoizedResults.has(cacheKey)) { + return memoizedResults.get(cacheKey) + } // ... existing computation ... + memoizedResults.set(cacheKey, instance) return instance }
Line range hint
941-1108: Enhance HMAC validation securityThe HMAC validation in
getSessionEvaluationcould be improved by using a timing-safe comparison to prevent timing attacks.Consider using the crypto module's timingSafeEqual:
+ import { timingSafeEqual } from 'crypto' if (typeof hmac === 'string') { const hmacEncoder = createHmac('sha256', process.env.APP_SECRET as string) hmacEncoder.update(session.namespace + session.id) const sessionHmac = hmacEncoder.digest('hex') - if (sessionHmac !== hmac) { + if (!timingSafeEqual(Buffer.from(sessionHmac), Buffer.from(hmac))) { return null } }apps/frontend-manage/src/components/courses/LiveQuizElement.tsx (2)
119-120: Update translation key to reflect 'live quiz' terminologyThe translation key
manage.sessions.nBlocksQuestionsuses 'sessions' instead of 'liveQuizzes', which may lead to confusion given the refactoring from sessions to live quizzes. Consider updating the translation key to reflect the new terminology.
119-120: Consistently update text and translation keys from 'session' to 'live quiz'There are instances where text and translation keys still reference 'session' instead of 'live quiz', which could cause confusion. For consistency, consider updating these references.
Examples:
- Line 119-120:
t('manage.sessions.nBlocksQuestions'- Line 143:
getAccessLinkusesquiz.name, ensure the associated text reflects 'live quiz'.- Line 150:
getLTIAccessLinkusesquiz.name, check the text for consistency.- Line 248: Deletion confirmation references 'deleteSession' in the translation key.
- Line 292: The
DeletionModalusest('manage.sessions.deleteLiveQuiz'), ensure all related text and keys are updated accordingly.Also applies to: 143-143, 150-150, 248-248, 292-292
packages/graphql/src/services/courses.ts (1)
712-720: Consider optimizing data retrieval forliveQuizzes.In the
includeclause forliveQuizzes, you're fetchingblocksand countingelementswithin each block. If not all of this data is necessary for your use case, consider selecting only the fields you need to improve query performance and reduce data transfer.
🛑 Comments failed to post (9)
packages/graphql/src/graphql/ops/QGetSingleCourse.graphql (1)
25-25: 💡 Codebase verification
Inconsistent renaming of 'sessions' to 'liveQuizzes' detected
The renaming is not consistently applied across the codebase. Found remaining instances of 'sessions':
packages/graphql/src/graphql/ops/QGetControlCourse.graphql: Still using 'sessions'packages/graphql/src/public/schema.graphql: Contains both 'sessions' and 'liveQuizzes' fieldsThis inconsistency could lead to breaking changes and schema conflicts. The transition from sessions to liveQuizzes should be applied uniformly across all related GraphQL operations and schema definitions.
🔗 Analysis chain
Verify consistent renaming of 'sessions' to 'liveQuizzes' across the codebase.
The field renaming aligns with the transition to quiz-based logic.
Let's verify the consistency of this change:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for any remaining references to 'sessions' that should have been renamed # Expect: No occurrences of old terminology in GraphQL files # Search for 'sessions' in GraphQL files echo "Checking for 'sessions' references in GraphQL files..." rg -t graphql 'sessions' # Search for potential GraphQL query/mutation references in TypeScript/JavaScript files echo "Checking for GetSingleCourse query usage..." rg 'GetSingleCourse.*sessions'Length of output: 426
Script:
#!/bin/bash # Let's try a different approach with proper file type specifications # Check for 'sessions' references in .graphql files echo "Checking for 'sessions' in .graphql files..." rg "sessions" --type-add 'graphql:*.graphql' -t graphql # Check for 'GetSingleCourse' query usage with 'sessions' echo "Checking for GetSingleCourse query usage with 'sessions'..." rg "GetSingleCourse.*sessions" -g "*.{ts,tsx,js,jsx}" # Check for 'liveQuizzes' to confirm the new terminology echo "Checking for 'liveQuizzes' in .graphql files..." rg "liveQuizzes" --type-add 'graphql:*.graphql' -t graphqlLength of output: 884
apps/frontend-manage/src/components/sessions/cockpit/CancelLiveQuizModal.tsx (1)
21-31: 🛠️ Refactor suggestion
Update prop name for consistency with quiz-based terminology.
The prop name
sessionIdshould be updated toquizIdto maintain consistency with the quiz-based terminology used throughout the codebase.function CancelLiveQuizModal({ - sessionId, + quizId, title, open, setOpen, }: { - sessionId: string + quizId: string title: string open: boolean setOpen: (value: boolean) => void })📝 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.function CancelLiveQuizModal({ quizId, title, open, setOpen, }: { quizId: string title: string open: boolean setOpen: (value: boolean) => void }) {apps/frontend-pwa/src/pages/course/[courseId]/liveQuizzes.tsx (2)
72-72:
⚠️ Potential issueFix inconsistent null checking for course data.
There's inconsistent handling of the course object between the Layout component and the course name display.
Apply consistent optional chaining:
- <Layout course={data.getCourseRunningLiveQuizzes[0].course ?? undefined}> + <Layout course={data.getCourseRunningLiveQuizzes[0].course}>Also applies to: 77-77
48-52: 🛠️ Refactor suggestion
Simplify redundant conditions.
The conditions for checking empty quiz list are redundant.
Simplify the conditions:
if ( isInactive || !data || - !data.getCourseRunningLiveQuizzes?.length || - data.getCourseRunningLiveQuizzes.length === 0 + !data.getCourseRunningLiveQuizzes?.length ) {📝 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 ( isInactive || !data || !data.getCourseRunningLiveQuizzes?.lengthapps/frontend-manage/src/pages/courses/[id].tsx (1)
162-179: 🛠️ Refactor suggestion
Update translation key to maintain consistency.
While the tab configuration is correctly updated for the quiz-based workflow, the translation key
t('manage.general.sessions')still references "sessions". This should be updated to maintain consistency with the new quiz-based terminology.Consider updating the translation key to reflect the new terminology:
-label={t('manage.general.sessions')} +label={t('manage.general.liveQuizzes')}📝 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.defaultValue="liveQuizzes" value={tabValue} onValueChange={(newValue: string) => setTabValue(newValue)} className={{ root: 'flex-1 basis-2/3' }} > <Tabs.TabList> <Tabs.Tab key="tab-liveQuizzes" value="liveQuizzes" label={t('manage.general.liveQuizzes')} className={{ root: 'border border-solid', label: twMerge( 'whitespace-nowrap text-base', tabValue === 'liveQuizzes' && 'font-bold' ), }} data={{ cy: 'tab-liveQuizzes' }}packages/graphql/src/services/liveQuizzes.ts (1)
519-787:
⚠️ Potential issueAddress potential race condition in
cancelLiveQuiz.The Redis cleanup and database updates are not atomic. Consider using Redis MULTI/EXEC to ensure atomicity:
Apply this diff:
- const keys = await ctx.redisExec.keys(`s:${id}:*`) - const pipe = ctx.redisExec.multi() - for (const key of keys) { - pipe.unlink(key) - } - await pipe.exec() + await ctx.redisExec + .multi() + .keys(`s:${id}:*`) + .exec((err, results) => { + if (err) throw err + const keys = results[0] + if (keys.length > 0) { + return ctx.redisExec.unlink(...keys) + } + })Committable suggestion skipped: line range outside the PR's diff.
apps/frontend-manage/src/components/courses/LiveQuizElement.tsx (2)
127-128:
⚠️ Potential issueHandle
PublicationStatus.Gradedin conditional renderingThe component currently does not handle the
PublicationStatus.Gradedstatus in the conditional rendering logic. To ensure that quizzes with the 'Graded' status are properly managed, consider adding conditions to handle this status and render appropriate UI elements.For example, you can update the conditions to include
PublicationStatus.Gradedwhere appropriate.- {(quiz.status === PublicationStatus.Scheduled || - quiz.status === PublicationStatus.Draft) && ( + {(quiz.status === PublicationStatus.Scheduled || + quiz.status === PublicationStatus.Draft || + quiz.status === PublicationStatus.Graded) && (And similarly for other status checks:
- {quiz.status === PublicationStatus.Ended && ( + {(quiz.status === PublicationStatus.Ended || + quiz.status === PublicationStatus.Graded) && (Ensure that the UI renders correctly for quizzes with the 'Graded' status.
Also applies to: 192-192, 229-229
104-104: 🛠️ Refactor suggestion
Update
data-cyattribute to use 'live-quiz' instead of 'session'The
data-cyattribute is currently using 'session', which is inconsistent with the updated terminology 'live-quiz'. For consistency and clarity, consider updating 'session' to 'live-quiz'.Apply this diff to fix the inconsistency:
- data-cy={`session-${quiz.name}`} + data-cy={`live-quiz-${quiz.name}`}📝 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.data-cy={`live-quiz-${quiz.name}`}packages/graphql/src/services/courses.ts (1)
793-798:
⚠️ Potential issueAdd null checks to prevent runtime errors when mapping
liveQuizzes.When mapping over
course?.liveQuizzes, ensure thatsession.blocksandblock._countare defined. Without null checks, there's a risk of encounteringTypeErrorif any of these arenullorundefined.Apply this diff to include safe navigation and default values:
const reducedLiveQuizzes = course?.liveQuizzes.map((session) => { return { ...session, numOfBlocks: session.blocks?.length ?? 0, numOfInstances: session.blocks?.reduce( (acc, block) => acc + (block._count?.elements ?? 0), 0 ), } })📝 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 reducedLiveQuizzes = course?.liveQuizzes.map((session) => { return { ...session, numOfBlocks: session.blocks?.length ?? 0, numOfInstances: session.blocks?.reduce( (acc, block) => acc + (block._count?.elements ?? 0), 0 ), } })
|
Summary by CodeRabbit
Release Notes
New Features
FeedbackAreaSubscribercomponent for real-time feedback updates during quizzes.changeLiveQuizSettingsmutation to adjust live quiz settings.CancelLiveQuizmutation for canceling live quizzes.GetCourseRunningLiveQuizzesquery to retrieve live quizzes associated with a specific course.Improvements
quizIdinstead ofsessionId, enhancing consistency across the application.LiveQuizElementandLiveQuizListcomponents to focus on live quizzes instead of sessions.CourseLiveQuizzescomponent for better user experience.Localization