refactor: replace question preview with student element and move artificial instance computation to backend #4357
Conversation
📝 WalkthroughWalkthroughThis pull request introduces significant changes primarily focused on updating type definitions related to student responses across various components and hooks. The type 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: 1
🧹 Outside diff range and nitpick comments (20)
packages/shared-components/src/hooks/useSingleStudentResponse.ts (1)
Line range hint
20-52: Consider refactoring for improved maintainability.While the implementation is functionally correct, we could improve its maintainability and readability:
- Extract the response initialization logic for each element type into separate functions
- Use a type guard or switch statement instead of type checking with
__typename- Consider using a map for element type handlers
Here's a suggested refactor:
type ResponseInitializer = ( instance: ElementInstance, defaultRead: boolean ) => InstanceStackStudentResponseType; const initializeChoicesResponse: ResponseInitializer = (instance) => ({ type: instance.elementData.type as ElementChoicesType, response: instance.elementData.options.choices.reduce( (acc, _, ix) => ({ ...acc, [ix]: undefined }), {} as Record<number, boolean | undefined> ), valid: false, }); const initializeContentResponse: ResponseInitializer = (instance, defaultRead) => ({ type: instance.elementData.type, response: defaultRead ? true : undefined, valid: true, }); const initializeDefaultResponse: ResponseInitializer = (instance) => ({ type: instance.elementData.type, response: undefined, valid: false, }); function useSingleStudentResponse({ instance, setStudentResponse, defaultRead = false, }: { instance?: ElementInstance | null setStudentResponse: React.Dispatch<React.SetStateAction<InstanceStackStudentResponseType>> defaultRead?: boolean }) { useEffect(() => { if (!instance) return; const handlers: Record<string, ResponseInitializer> = { ChoicesElementData: initializeChoicesResponse, [ElementType.Content]: initializeContentResponse, }; const handler = handlers[ instance.elementData.__typename ?? instance.elementData.type ] ?? initializeDefaultResponse; setStudentResponse(handler(instance, defaultRead)); }, [instance]); }packages/shared-components/src/hooks/useStudentResponse.ts (1)
Line range hint
26-65: Consider enhancing element type handling with a type guard.The current implementation uses type checking with
__typenameandtype. Consider using a type guard to improve type safety and readability.Here's a suggested improvement:
+ type ElementDataType = ElementStack['elements'][0]['elementData'] + + function isChoicesElement(data: ElementDataType): data is Extract<ElementDataType, { __typename: 'ChoicesElementData' }> { + return data.__typename === 'ChoicesElementData' + } + + function isContentElement(data: ElementDataType): data is Extract<ElementDataType, { type: ElementType.Content }> { + return data.type === ElementType.Content + } useEffect(() => { const newStudentResponse = stack.elements?.reduce((acc, element) => { - if (element.elementData.__typename === 'ChoicesElementData') { + if (isChoicesElement(element.elementData)) { // ... rest of the choices handling - } else if (element.elementData.type === ElementType.Content) { + } else if (isContentElement(element.elementData)) { // ... rest of the content handling }apps/frontend-pwa/src/components/hooks/useStackEvaluationAggregation.ts (1)
Line range hint
1-93: Consider adding error handling for localStorage parsing.While the overall implementation looks good, the JSON parsing of localStorage data could fail if the stored data is malformed.
Consider wrapping the parse in a try-catch:
- const stackStorage: StackStudentResponseType = rawStackStorage - ? JSON.parse(rawStackStorage) - : null + let stackStorage: StackStudentResponseType = null + try { + stackStorage = rawStackStorage ? JSON.parse(rawStackStorage) : null + } catch (e) { + console.error('Failed to parse stack storage:', e) + // Continue with null storage, which is handled by existing logic + }apps/frontend-manage/src/components/questions/manipulation/StudentElementPreview.tsx (1)
Line range hint
34-123: Consider refactoring for improved maintainabilityThe element data transformation logic contains complex nested conditionals and type assertions. Consider these improvements:
- Extract the element data transformation into a separate utility function
- Replace type assertions with proper type guards
- Simplify nested conditional checks using helper functions
Example refactor for the element data transformation:
interface ElementDataTransformProps { values: ElementFormTypes; elementDataTypename: ElementData['__typename']; } function transformToElementInstance({ values, elementDataTypename }: ElementDataTransformProps): ElementInstance { const options = 'options' in values ? transformOptions(values.options) : undefined; return { id: 0, type: ElementInstanceType.LiveQuiz, elementType: values.type, elementData: { id: '0', elementId: 0, __typename: elementDataTypename, content: values.content, explanation: 'explanation' in values ? values.explanation : undefined, name: values.name, pointsMultiplier: parseInt(values.pointsMultiplier ?? '1'), type: values.type, options, }, }; }apps/frontend-pwa/src/components/liveSession/QuestionArea.tsx (2)
115-116: Consider enhancing error handling for invalid inputs.While the type update is correct, the function could benefit from more robust error handling:
- Consider logging invalid inputs for debugging
- Add error feedback for invalid responses
input: InstanceStackStudentResponseType }): void => { if (!input.valid) { + console.warn('Invalid input received:', input); + // Consider adding user feedback here return; }
Line range hint
177-177: Address TODO comment regarding failed response handling.The current implementation might leave the system in an inconsistent state if some responses are saved while others fail. Consider implementing a transaction-like approach or rollback mechanism.
Would you like me to propose a solution for handling failed response saves more robustly?
packages/shared-components/src/StudentElement.tsx (3)
Line range hint
20-55: LGTM! Consider adding JSDoc comments.The type definitions are well-structured and comprehensive, covering all element types with their respective response formats. The naming convention clearly indicates the purpose and hierarchy of types.
Consider adding JSDoc comments to document the purpose and usage of these types, especially since they're exported:
+/** + * Represents a student's response for a single instance within a stack, + * including the response data, validation state, and evaluation results. + */ export type InstanceStackStudentResponseType = { // ... existing type definition } +/** + * Maps element IDs to their corresponding student responses within a stack. + */ export type StackStudentResponseType = Record< number, InstanceStackStudentResponseType >
65-67: LGTM! Consider adding JSDoc comments.The stack-related props are well-typed and maintain type safety with the new type system.
Consider adding JSDoc comments to document the props:
+/** + * Props for handling student responses within a stack context. + */ interface StudentElementStackProps extends StudentElementBaseProps { + /** Current state of student responses across the stack */ studentResponse: StackStudentResponseType + /** Setter for updating student responses */ setStudentResponse: Dispatch<SetStateAction<StackStudentResponseType>> + /** Optional storage for persisted responses */ stackStorage?: StackStudentResponseType // ... rest of the interface }
76-79: Consider renamingsingleStudentResponsefor clarity.The prop name
singleStudentResponsemight be misleading as it uses an instance from a stack. Consider a more explicit name to reflect its purpose.- singleStudentResponse: InstanceStackStudentResponseType - setSingleStudentResponse: Dispatch< - SetStateAction<InstanceStackStudentResponseType> - > + instanceResponse: InstanceStackStudentResponseType + setInstanceResponse: Dispatch< + SetStateAction<InstanceStackStudentResponseType> + >apps/frontend-pwa/src/components/groupActivity/GroupActivityStack.tsx (3)
Line range hint
72-251: Add error handling for numerical responsesThe numerical response parsing could fail with invalid input, but there's no error handling.
Consider adding validation:
} else if (value.type === ElementType.Numerical) { + const numValue = parseFloat(value.response!) + if (isNaN(numValue)) { + console.error('Invalid numerical response') + return acc + } return { instanceId: parseInt(instanceId), type: ElementType.Numerical, - numericalResponse: parseFloat(value.response!), + numericalResponse: numValue, } }
Line range hint
224-230: Fix typo in contentResponse property nameThere's a typo in the property name 'contentReponse' (missing 's').
Apply this fix:
return { instanceId: parseInt(instanceId), type: ElementType.Content, - contentReponse: value.response, + contentResponse: value.response, }
Line range hint
72-251: Consider refactoring response processing logicThe response processing logic is complex with nested conditionals. Consider extracting type-specific processing into separate functions for better maintainability.
Example refactor:
const processChoiceResponse = (instanceId: number, value: StackStudentResponseType[string], type: ElementType) => { const responseList = Object.entries(value.response!) .filter(([, value]) => value) .map(([key]) => parseInt(key)) return { instanceId: parseInt(instanceId), type, choicesResponse: responseList, } } // Similar functions for other types const processNumericalResponse = (instanceId: number, value: StackStudentResponseType[string]) => { const numValue = parseFloat(value.response!) if (isNaN(numValue)) { console.error('Invalid numerical response') return null } return { instanceId: parseInt(instanceId), type: ElementType.Numerical, numericalResponse: numValue, } } // Then in the submit handler: const responses = Object.entries(studentResponse) .map(([instanceId, value]) => { switch (value.type) { case ElementType.Sc: case ElementType.Mc: case ElementType.Kprim: return processChoiceResponse(instanceId, value, value.type) case ElementType.Numerical: return processNumericalResponse(instanceId, value) // ... other cases } }) .filter(Boolean) // Remove null responsesapps/frontend-manage/src/components/courses/groupActivity/GroupActivityGradingStack.tsx (2)
Line range hint
64-121: Consider enhancing type safety in findResponse function.The
findResponsefunction handles multiple element types with complex type-specific logic. Consider these improvements:
- Add explicit return type annotation
- Use discriminated unions for element types
- Extract type-specific response handling into separate functions
Example refactor:
type ResponseResult = { [key: number]: { type: ElementType; response: any; // Replace with specific types valid: boolean; }; }; const findResponse = useCallback( (elementId: number, type: ElementType): ResponseResult | undefined => { // ... existing code }, [submission?.decisions] );
Line range hint
367-386: Consider enhancing error handling in form submission.The error toast could provide more specific information about what went wrong during submission.
Example enhancement:
if (result.data?.gradeGroupActivitySubmission?.id) { setSubmitting(false); resetForm(); setSuccessToast(true); setEdited(false); } else { setSubmitting(false); setErrorToast(true); // Add error details console.error('Failed to grade submission:', result.errors); // Consider showing specific error message // setErrorMessage(result.errors?.[0]?.message ?? 'Unknown error occurred'); }apps/frontend-pwa/src/components/practiceQuiz/ElementStack.tsx (1)
Line range hint
131-456: Consider extracting complex reduce operations into utility functions.The reduce operations are type-safe but complex. Consider extracting them into separate utility functions for better maintainability and testability. For example:
const processEvaluations = (evaluations: Evaluation[], stack: Stack): StackStudentResponseType => { return evaluations.reduce<StackStudentResponseType>((acc, evaluation) => { // existing evaluation processing logic }, {}) } const processContentResponses = (currentResponses: StackStudentResponseType): StackStudentResponseType => { return Object.entries(currentResponses).reduce<StackStudentResponseType>((acc, [instanceId, value]) => { // existing content processing logic }, {}) } const processSubmissionResponses = ( studentResponse: StackStudentResponseType, evaluations: Evaluation[] ): StackStudentResponseType => { return Object.entries(studentResponse).reduce<StackStudentResponseType>((acc, [key, value]) => { // existing submission processing logic }, {}) }packages/graphql/src/schema/query.ts (1)
494-503: Consider adding input validation and error handling.The implementation looks good and aligns with the PR objective of moving artificial instance computation to backend. However, consider enhancing the resolver with:
- Input validation to ensure elementId exists
- Proper error handling for cases where:
- Element doesn't exist
- User doesn't have access to the element
- Service method fails
Example enhancement:
artificialInstance: asUser.field({ nullable: true, type: ElementInstance, args: { elementId: t.arg.int({ required: true }), }, - resolve(_, args, ctx) { - return QuestionService.getArtificialElementInstance(args, ctx) + async resolve(_, args, ctx) { + try { + // Verify element exists and user has access + const element = await ctx.prisma.element.findUnique({ + where: { id: args.elementId }, + }) + if (!element) { + throw new Error('Element not found') + } + return QuestionService.getArtificialElementInstance(args, ctx) + } catch (error) { + console.error('Error in artificialInstance resolver:', error) + throw error + } + } }),packages/graphql/src/services/questions.ts (1)
162-182: Consider using constants for default values.The function contains several hardcoded values that would be better defined as named constants, especially if they're used elsewhere in the codebase.
Consider extracting these into named constants:
+const DEFAULT_ARTIFICIAL_INSTANCE = { + id: 0, + order: 0, + type: DB.ElementInstanceType.LIVE_QUIZ, + migrationId: '', + originalId: '', + ownerId: '', + elementBlockId: 0, + elementStackId: null, +} as const; return { - id: 0, - elementId: element.id, - migrationId: '', - originalId: '', + ...DEFAULT_ARTIFICIAL_INSTANCE, + elementId: element.id, elementType: element.type, - order: 0, - type: DB.ElementInstanceType.LIVE_QUIZ, elementData, options: { pointsMultiplier: element.pointsMultiplier, }, results: initialResults, anonymousResults: initialResults, - ownerId: '', - elementBlockId: 0, - elementStackId: null, createdAt: new Date(), updatedAt: new Date(), }packages/graphql/src/public/schema.graphql (1)
1403-1403: Add description for the new query field.The new query field looks good and aligns with the PR objectives of moving artificial instance computation to the backend. Consider adding a description field to document its purpose and usage.
Apply this diff to add documentation:
+ """ + Retrieves an artificial element instance for preview purposes based on the provided element ID. + """ artificialInstance(elementId: Int!): ElementInstancepackages/graphql/src/ops.schema.json (2)
20320-20348: Add descriptions for better API documentation.The new
artificialInstancequery field is well-structured, but lacks descriptions that would help API consumers understand its purpose and usage.Consider adding descriptions:
{ "name": "artificialInstance", - "description": null, + "description": "Retrieves an artificial instance for preview purposes based on the given element ID", "args": [ { "name": "elementId", - "description": null, + "description": "The unique identifier of the element for which to generate an artificial instance", "type": { "kind": "NON_NULL",
20324-20339: Consider adding input validation for elementId.While the
elementIdis properly typed as a non-null Int, consider adding validation constraints to ensure it's a positive number.You might want to:
- Add a custom scalar type for positive integers
- Document the valid range for elementId
- Implement validation in the resolver
This would help prevent invalid requests and improve API robustness.
🛑 Comments failed to post (1)
packages/graphql/src/services/questions.ts (1)
142-182: 🛠️ Refactor suggestion
Add TypeScript return type and documentation.
The new function lacks a return type definition and documentation explaining its purpose and usage.
Add TypeScript type and JSDoc:
+/** + * Creates an artificial element instance for preview purposes. + * @param elementId - The ID of the element to create an instance from + * @param ctx - Context containing user information + * @returns ElementInstance or null if element not found + */ -export async function getArtificialElementInstance( +export async function getArtificialElementInstance( { elementId, }: { elementId: number }, - ctx: ContextWithUser + ctx: ContextWithUser +): Promise<DB.ElementInstance | 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./** * Creates an artificial element instance for preview purposes. * @param elementId - The ID of the element to create an instance from * @param ctx - Context containing user information * @returns ElementInstance or null if element not found */ export async function getArtificialElementInstance( { elementId, }: { elementId: number }, ctx: ContextWithUser ): Promise<DB.ElementInstance | null> { const element = await ctx.prisma.element.findUnique({ where: { id: elementId, ownerId: ctx.user.sub, }, }) if (!element) return null const elementData = processElementData(element) const initialResults = getInitialElementResults(element) return { id: 0, elementId: element.id, migrationId: '', originalId: '', elementType: element.type, order: 0, type: DB.ElementInstanceType.LIVE_QUIZ, elementData, options: { pointsMultiplier: element.pointsMultiplier, }, results: initialResults, anonymousResults: initialResults, ownerId: '', elementBlockId: 0, elementStackId: null, createdAt: new Date(), updatedAt: new Date(), } }
klicker-uzh
|
||||||||||||||||||||||||||||
| Project |
klicker-uzh
|
| Branch Review |
replace-question-preview
|
| Run status |
|
| Run duration | 11m 08s |
| 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 ↗︎ | |
Summary by CodeRabbit
Release Notes
New Features
GetArtificialInstancefor retrieving detailed artificial instance data.QuestionAreacomponent to support a new data structure for student responses.Improvements
QuestionDetailscomponent for better user experience.Bug Fixes
Chores
useFakedInstancehook as it was no longer needed.