enhance: allow incomplete submissions for selection questions #4453
Conversation
📝 WalkthroughWalkthroughThe pull request introduces a comprehensive modification to how selection responses are handled across multiple components and services. The changes primarily focus on refining the filtering and validation logic for selection responses, specifically introducing a consistent approach to handling skipped or invalid selections by using Changes
Possibly related PRs
Suggested reviewers
Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
packages/shared-components/src/utils/validateResponse.ts (1)
95-97: Consider adding a constant for the invalid selection marker.The magic number
-1is used to represent invalid/skipped selections. Consider defining it as a named constant for better maintainability and clarity.+const INVALID_SELECTION = -1; export function validateSelectionResponse({ response, }: { response?: Record<number, number> }) { if ( !response || - Object.values(response).every((value) => value === -1) || + Object.values(response).every((value) => value === INVALID_SELECTION) || new Set(Object.values(response).filter((r) => r !== -1)).size !== - Object.values(response).filter((r) => r !== -1).length + Object.values(response).filter((r) => r !== INVALID_SELECTION).length ) { return false }packages/shared-components/src/questions/SELECTIONAnswerOptions.tsx (1)
27-29: Consider extracting the filter condition to a utility function.The filtering logic for selected values could be moved to a shared utility function to maintain consistency across components.
+const isValidSelection = (value: number) => value !== -1; // get the selected options, which are not unselected -const selectedValues = Object.values(responses).filter( - (selectedValue) => selectedValue !== -1 -) +const selectedValues = Object.values(responses).filter(isValidSelection)apps/frontend-pwa/src/components/groupActivity/GroupActivityStack.tsx (1)
290-293: Simplify the selection response filtering.Given the type changes to
Record<number, number>, checking forundefinedmight be redundant. Consider simplifying the filter condition.-selectionResponse: Object.values( - value.response! -).filter( - (entry) => - typeof entry !== 'undefined' && entry !== -1 -) +selectionResponse: Object.values(value.response!).filter((entry) => entry !== -1)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
apps/frontend-pwa/src/components/groupActivity/GroupActivityStack.tsx(1 hunks)apps/func-response-processor/src/index.ts(2 hunks)packages/graphql/src/services/stacks.ts(1 hunks)packages/shared-components/src/StudentElement.tsx(2 hunks)packages/shared-components/src/questions/SELECTIONAnswerOptions.tsx(3 hunks)packages/shared-components/src/utils/validateResponse.ts(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: check
- GitHub Check: SonarCloud
- GitHub Check: cypress-run
- GitHub Check: build
- GitHub Check: Analyze (javascript)
- GitHub Check: build
- GitHub Check: test
🔇 Additional comments (8)
packages/shared-components/src/utils/validateResponse.ts (1)
90-90: LGTM! Type signature change enhances type safety.The change from
Record<number, number | undefined>toRecord<number, number>provides better type safety by using-1as an explicit marker for skipped selections instead ofundefined.packages/shared-components/src/questions/SELECTIONAnswerOptions.tsx (2)
9-10: LGTM! Props interface changes maintain type consistency.The changes to
responsesandonChangetypes align with the new approach of using numbers instead of undefined values.
85-88: LGTM! Select component value handling is consistent.The value handling in the Select component correctly manages the -1 case for unselected options.
packages/shared-components/src/StudentElement.tsx (2)
54-54: LGTM! Type definition change maintains consistency.The update to
InstanceStackStudentResponseTypefor Selection case aligns with the new approach of using numbers instead of undefined values.
293-294: LGTM! Type casting is correctly updated.The type casting in the SelectionQuestion component properly reflects the type changes in the response handling.
apps/func-response-processor/src/index.ts (2)
364-368: LGTM! Early return for skipped input fields.The conditional block correctly prevents processing of skipped input fields (marked with
-1), which aligns with the PR objective of allowing incomplete submissions.
376-376: Consistent filtering of skipped responses.The implementation consistently filters out skipped responses (
-1values) in both:
- The grading function input (line 376)
- The Redis storage (line 416)
This ensures that skipped responses are handled uniformly throughout the processing pipeline.
Also applies to: 416-416
packages/graphql/src/services/stacks.ts (1)
2794-2796: LGTM! Consistent handling of skipped responses.The filtering of
-1values before forwarding the selection response aligns with the changes in the response processor, ensuring consistent handling of skipped responses throughout the application.
klicker-uzh
|
||||||||||||||||||||||||||||
| Project |
klicker-uzh
|
| Branch Review |
v3-new-elements
|
| Run status |
|
| Run duration | 17m 30s |
| Commit |
|
| Committer | Julius Schlapbach |
| View all properties for this run ↗︎ | |
| Test results | |
|---|---|
Failures
|
0
|
Flaky
|
0
|
Pending
|
0
|
Skipped
|
0
|
Passing
|
252
|
| View all changes introduced in this branch ↗︎ | |
klicker-uzh
|
||||||||||||||||||||||||||||
| Project |
klicker-uzh
|
| Branch Review |
selection-incomplete-submission
|
| Run status |
|
| Run duration | 16m 34s |
| Commit |
|
| Committer | Julius Schlapbach |
| View all properties for this run ↗︎ | |
| Test results | |
|---|---|
|
Failures
|
0
|
|
Flaky
|
0
|
|
Pending
|
0
|
|
Skipped
|
0
|
|
Passing
|
254
|
| View all changes introduced in this branch ↗︎ | |
…ews of selection question
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
packages/shared-components/src/utils/getEmptySelectionResponse.ts (1)
1-11: LGTM! Consider adding input validation.The implementation is clean and focused. Using -1 as a marker for empty/invalid selections is a good pattern.
Consider adding validation for negative numberOfInputs:
function getEmptySelectionResponse({ numberOfInputs, }: { numberOfInputs?: number | null }) { + const count = Math.max(0, numberOfInputs ?? 0) const initResponses: Record<number, number> = {} - for (let i = 0; i < (numberOfInputs ?? 0); i++) { + for (let i = 0; i < count; i++) { initResponses[i] = -1 } return initResponses }packages/shared-components/src/SelectionQuestion.tsx (1)
41-57: LGTM! Consider extracting the response initialization logic.The implementation is robust with proper memoization and type safety.
Consider extracting the response initialization logic to improve readability:
+ const getInitialResponse = ( + completedExisting: Record<number, number> | undefined, + current: Record<number, number> | undefined, + empty: Record<number, number> + ) => completedExisting ?? current ?? empty const emptyResponses = useMemo( () => getEmptySelectionResponse({ numberOfInputs: options.numberOfInputs, }), [options.numberOfInputs] ) const completedExistingResponse = useMemo( () => completeSelectionResponse({ existingResponse, emptyResponses, }), [existingResponse, emptyResponses] )Then use it in the SELECTIONAnswerOptions:
- responses={completedExistingResponse ?? response ?? emptyResponses} + responses={getInitialResponse(completedExistingResponse, response, emptyResponses)}apps/frontend-pwa/src/components/groupActivity/GroupActivityStack.tsx (2)
128-141: Consider extracting instance lookup for safer handling.While the implementation is functional, the instance lookup and access could be made more robust.
Consider this refactor for better null safety:
- const instance = stack.elements?.find( - (element) => element.id === decision.instanceId - ) - const response = getEmptySelectionResponse({ - numberOfInputs: instance - ? (instance.elementData as SelectionElementData).options - .numberOfInputs - : 1, - }) + const instance = stack.elements?.find( + (element) => element.id === decision.instanceId + ) + const numberOfInputs = instance + ? (instance.elementData as SelectionElementData).options.numberOfInputs + : 1 + const response = getEmptySelectionResponse({ numberOfInputs })
307-310: Optimize filtering performance.The filtering logic is correct but could be optimized to avoid O(n²) complexity from spread operators.
Consider this optimization:
- selectionResponse: Object.values( - value.response! - ).filter( - (entry) => - typeof entry !== 'undefined' && entry !== -1 - ), + selectionResponse: Object.values(value.response!) + .reduce((acc: number[], entry) => { + if (typeof entry !== 'undefined' && entry !== -1) { + acc.push(entry) + } + return acc + }, []),
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
apps/frontend-manage/src/components/courses/groupActivity/GroupActivityGradingStack.tsx(5 hunks)apps/frontend-pwa/src/components/groupActivity/GroupActivityStack.tsx(4 hunks)packages/shared-components/src/SelectionQuestion.tsx(3 hunks)packages/shared-components/src/hooks/useStudentResponse.ts(2 hunks)packages/shared-components/src/utils/completeSelectionResponse.ts(1 hunks)packages/shared-components/src/utils/getEmptySelectionResponse.ts(1 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
packages/shared-components/src/hooks/useStudentResponse.ts
[error] 60-60: Avoid the use of spread (...) syntax on accumulators.
Spread syntax should be avoided on accumulators (like those in .reduce) because it causes a time complexity of O(n^2).
Consider methods such as .splice or .push instead.
(lint/performance/noAccumulatingSpread)
apps/frontend-pwa/src/components/groupActivity/GroupActivityStack.tsx
[error] 144-144: Avoid the use of spread (...) syntax on accumulators.
Spread syntax should be avoided on accumulators (like those in .reduce) because it causes a time complexity of O(n^2).
Consider methods such as .splice or .push instead.
(lint/performance/noAccumulatingSpread)
⏰ Context from checks skipped due to timeout of 90000ms (8)
- GitHub Check: test
- GitHub Check: check
- GitHub Check: build
- GitHub Check: build
- GitHub Check: Analyze (javascript)
- GitHub Check: cypress-run
- GitHub Check: SonarCloud
- GitHub Check: build
🔇 Additional comments (5)
packages/shared-components/src/SelectionQuestion.tsx (1)
77-77: Verify the response initialization chain.The response fallback chain (
completedExistingResponse ?? response ?? emptyResponses) looks correct, but let's verify that this order maintains the intended behavior across the codebase.✅ Verification successful
Response initialization chain verified as correct.
The chain
completedExistingResponse ?? response ?? emptyResponsesmaintains the intended behavior by:
- Prioritizing completed existing responses
- Falling back to current responses
- Defaulting to empty responses (-1 values) when no other responses exist
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for other instances of response initialization patterns rg -A 2 "completedExistingResponse.*response.*emptyResponses" . # Search for any direct usage of these variables rg "\b(completedExistingResponse|response|emptyResponses)\b"Length of output: 80725
Script:
#!/bin/bash # Find implementation of emptyResponses and completedExistingResponse ast-grep --pattern 'const emptyResponses = useMemo($$$)' ast-grep --pattern 'const completedExistingResponse = useMemo($$$)' # Look for any utility functions related to response completion fd completeResponse . --exec rg -A 5 "function.*completeResponse" # Look for similar fallback patterns with nullish coalescing rg "\?\?" . --type ts --type tsxLength of output: 1460
Script:
#!/bin/bash # Find implementation of utility functions fd -e ts -e tsx getEmptySelectionResponse . --exec rg -A 10 "export.*getEmptySelectionResponse" fd -e ts -e tsx completeSelectionResponse . --exec rg -A 10 "export.*completeSelectionResponse" # Look for similar patterns with nullish coalescing (fixed extension) rg "\?\?" . -t ts -t tsx -A 2 # Look for any other response initialization patterns rg "response.*\?\?" . -t ts -t tsxLength of output: 402
Script:
#!/bin/bash # Find implementation of utility functions using ast-grep ast-grep --pattern 'function getEmptySelectionResponse($$$) { $$$ }' ast-grep --pattern 'function completeSelectionResponse($$$) { $$$ }' # Find and show the utility files fd -e ts -e tsx "completeSelectionResponse|getEmptySelectionResponse" . --exec cat {} # Look for similar patterns fd -e ts -e tsx . --exec grep -A 2 "??" {} \;Length of output: 66097
apps/frontend-manage/src/components/courses/groupActivity/GroupActivityGradingStack.tsx (3)
5-5: LGTM! Import statements are correctly added.The new imports support the enhanced selection response handling functionality.
Also applies to: 12-12, 17-17
72-72: LGTM! Function signature update is appropriate.The addition of the
elementDataparameter enables access to selection element configuration, which is necessary for handling selection responses.
130-139: LGTM! Selection response handling is well-implemented.The implementation correctly:
- Initializes the response array with proper length using
getEmptySelectionResponse- Preserves existing selections while allowing incomplete submissions
- Aligns with the PR objective
apps/frontend-pwa/src/components/groupActivity/GroupActivityStack.tsx (1)
9-9: LGTM! Import statements are correctly added.The new imports maintain consistency with the changes in GroupActivityGradingStack.
Also applies to: 17-17
packages/shared-components/src/utils/completeSelectionResponse.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
cypress/cypress/e2e/G-microlearning-workflow.cy.ts (1)
1113-1113: Add a descriptive comment for the finish step.Line 1113 should have a comment describing the purpose of the finish step, similar to other sections.
- // + // finish the microlearning and verify completioncypress/cypress/e2e/J-group-activity-workflow.cy.ts (1)
654-687: Consider reducing code duplication in verification functions.The
checkPersistentAnswersPartial()function shares significant code withcheckPersistentAnswers(). Consider extracting common assertions into a shared helper function.+function checkCommonAssertions() { + cy.get('[data-cy="sc-1-answer-option-1"]').should('be.disabled') + cy.get('[data-cy="sc-1-answer-option-2"]').should('be.disabled') + // ... other common assertions +} function checkPersistentAnswers() { - cy.get('[data-cy="sc-1-answer-option-1"]').should('be.disabled') - cy.get('[data-cy="sc-1-answer-option-2"]').should('be.disabled') + checkCommonAssertions() // ... specific full submission assertions } function checkPersistentAnswersPartial() { - cy.get('[data-cy="sc-1-answer-option-1"]').should('be.disabled') - cy.get('[data-cy="sc-1-answer-option-2"]').should('be.disabled') + checkCommonAssertions() // ... specific partial submission assertions }cypress/cypress/e2e/H-practice-quiz-workflow.cy.ts (3)
Line range hint
532-695: Refactor to improve maintainability and reduce code duplication.Consider the following improvements:
- Extract repeated disabled state assertions into helper functions.
- Move hardcoded test data to constants.
Example refactor:
+ const assertDisabledOptions = (type: string, count: number) => { + for (let i = 1; i <= count; i++) { + cy.get(`[data-cy="${type}-1-answer-option-${i}"]`).should('be.disabled') + } + } function answerRunningPracticeQuiz() { // ... other code ... // SC question - cy.get('[data-cy="sc-1-answer-option-1"]').should('be.disabled') - cy.get('[data-cy="sc-1-answer-option-2"]').should('be.disabled') - cy.get('[data-cy="sc-1-answer-option-3"]').should('be.disabled') - cy.get('[data-cy="sc-1-answer-option-4"]').should('be.disabled') + assertDisabledOptions('sc', 4) // ... similar refactoring for other question types ... }
Line range hint
697-814: Well-structured implementation with clear documentation.The function effectively tests partial submissions with clear comments indicating supported question types. Consider applying the same refactoring suggestions as
answerRunningPracticeQuizto maintain consistency.
858-863: Consider adding persistence verification.The test effectively verifies partial submissions, but consider adding assertions to verify that:
- Partial submissions are correctly persisted in the database
- Partial submissions can be resumed later
Example additions:
it('Verify persistence of partial submissions', () => { // Log out and back in to verify persistence cy.logout() cy.loginStudentPassword({ username: Cypress.env('STUDENT_USERNAME2') }) cy.get('[data-cy="quizzes"]').click() cy.get(`[data-cy="practice-quiz-${runningDisplayName}"]`).click() // Verify previously submitted answers are still present and disabled cy.get('[id="selection-1-field-1"]') .contains(SECollectionOptions[0]) .should('have.css', 'pointer-events', 'none') })
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
cypress/cypress/e2e/F-live-quiz-workflow.cy.ts(2 hunks)cypress/cypress/e2e/G-microlearning-workflow.cy.ts(4 hunks)cypress/cypress/e2e/H-practice-quiz-workflow.cy.ts(7 hunks)cypress/cypress/e2e/J-group-activity-workflow.cy.ts(5 hunks)packages/shared-components/src/hooks/useStudentResponse.ts(2 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
packages/shared-components/src/hooks/useStudentResponse.ts
[error] 60-60: Avoid the use of spread (...) syntax on accumulators.
Spread syntax should be avoided on accumulators (like those in .reduce) because it causes a time complexity of O(n^2).
Consider methods such as .splice or .push instead.
(lint/performance/noAccumulatingSpread)
⏰ Context from checks skipped due to timeout of 90000ms (8)
- GitHub Check: build
- GitHub Check: cypress-run
- GitHub Check: check
- GitHub Check: SonarCloud
- GitHub Check: Analyze (javascript)
- GitHub Check: build
- GitHub Check: format
- GitHub Check: build
🔇 Additional comments (11)
packages/shared-components/src/hooks/useStudentResponse.ts (3)
1-2: LGTM! Import statements are well-organized.The new imports for
SelectionElementDataandgetEmptySelectionResponseare correctly placed and necessary for the Selection type handling implementation.Also applies to: 8-8
53-67: Previous review comments about performance and type safety are still applicable.The Selection type handling implementation needs improvements in:
- Type safety: Replace type casting with type guard
- Performance: Avoid spread operator in reducer accumulator
The previous review comment provided a solution that addresses both issues. Please implement those changes to improve the code quality and performance.
🧰 Tools
🪛 Biome (1.9.4)
[error] 60-60: Avoid the use of spread (
...) syntax on accumulators.Spread syntax should be avoided on accumulators (like those in
.reduce) because it causes a time complexity ofO(n^2).
Consider methods such as .splice or .push instead.(lint/performance/noAccumulatingSpread)
53-67: Implementation aligns well with PR objectives.The approach to handling Selection type elements is consistent with other element types and correctly supports incomplete submissions by:
- Initializing empty responses
- Setting valid flag appropriately
- Maintaining consistent response structure
🧰 Tools
🪛 Biome (1.9.4)
[error] 60-60: Avoid the use of spread (
...) syntax on accumulators.Spread syntax should be avoided on accumulators (like those in
.reduce) because it causes a time complexity ofO(n^2).
Consider methods such as .splice or .push instead.(lint/performance/noAccumulatingSpread)
cypress/cypress/e2e/G-microlearning-workflow.cy.ts (4)
15-16: Good use of constants for test data!Extracting test input values into constants improves maintainability and reusability across test cases.
Line range hint
910-936: Well-structured test helper functions!The functions
enterValidCompleteInputsandverifyPersistentCompleteInputseffectively:
- Follow DRY principle by centralizing common test logic
- Provide comprehensive validation for all question types
- Maintain clear separation between input and verification steps
Also applies to: 938-978
1023-1028: Address the TODO for backend refetch testing.The commented-out test code for verifying backend refetch functionality should be implemented to ensure data persistence across sessions.
Would you like me to help implement the backend refetch verification test?
Line range hint
980-1034: Comprehensive test coverage for response scenarios!The test implementation effectively covers:
- Complete response submission and verification
- Partial response handling where supported
- Input persistence across page reloads
Also applies to: 1036-1095
cypress/cypress/e2e/J-group-activity-workflow.cy.ts (2)
567-583: Well-structured implementation of partial answer submission!The new
answerGroupActivityPartial()function effectively implements the partial submission feature while maintaining consistency with the existing codebase.
Line range hint
847-868: Well-structured test case for partial submissions!The test case effectively validates the new partial submission feature while maintaining consistency with the existing test suite structure.
cypress/cypress/e2e/F-live-quiz-workflow.cy.ts (2)
935-939: LGTM! Test case name accurately reflects the new functionality.The updated test case name clearly indicates that it tests partial answer submission support.
939-972: Well-documented test cases with clear behavior specifications.Good addition of comments that clearly document which question types do not support partial answers or skipping. This improves test maintainability and serves as documentation.
…er side response loading for microlearnins
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
packages/shared-components/src/hooks/useStudentResponse.ts (1)
29-39: 🛠️ Refactor suggestionImprove performance of the choices handler.
Using spread operator in the reducer accumulator could lead to O(n²) complexity.
- return { ...acc, [choice.ix]: undefined } + acc[choice.ix] = undefined + return acc🧰 Tools
🪛 Biome (1.9.4)
[error] 34-34: Avoid the use of spread (
...) syntax on accumulators.Spread syntax should be avoided on accumulators (like those in
.reduce) because it causes a time complexity ofO(n^2).
Consider methods such as .splice or .push instead.(lint/performance/noAccumulatingSpread)
🧹 Nitpick comments (8)
packages/shared-components/src/hooks/useStudentResponse.ts (2)
48-59: Remove unnecessary type casting.Since we've already checked
__typename === 'SelectionElementData', TypeScript should narrow the type automatically. The type casting is redundant and can be removed.} else if (element.elementData.__typename === 'SelectionElementData') { const emptyResponses = getEmptySelectionResponse({ - numberOfInputs: (element.elementData as SelectionElementData) - .options.numberOfInputs, + numberOfInputs: element.elementData.options.numberOfInputs, })
62-69: Move inline comment to JSDoc for better documentation.The comment about supported element types in the default case should be moved to JSDoc format above the else block for better code documentation.
+ /** + * Default case - handles the following element types: + * - FREE_TEXT + * - NUMERICAL + * - FLASHCARD + */ - // default case - valid for FREE_TEXT, NUMERICAL, FLASHCARD elements else {cypress/cypress/e2e/G-microlearning-workflow.cy.ts (4)
938-978: Consider reducing verification code duplication.While the verification function is thorough, there's repetitive code for checking disabled states. Consider extracting common verification patterns into helper functions.
Example refactor:
+function verifyDisabledState(selector: string) { + cy.get(selector).should('be.disabled') +} + +function verifyDisabledStateWithValue(selector: string, value: string) { + cy.get(selector) + .should('have.value', value) + .should('be.disabled') +} + function verifyPersistentCompleteInputs() { - cy.get('[data-cy="sc-1-answer-option-1"]').should('be.disabled') - cy.get('[data-cy="sc-1-answer-option-2"]').should('be.disabled') + verifyDisabledState('[data-cy="sc-1-answer-option-1"]') + verifyDisabledState('[data-cy="sc-1-answer-option-2"]') // ... similar refactoring for other verifications }
1036-1053: Consider extracting common input logic.The
enterValidPartialInputsfunction shares significant code withenterValidCompleteInputs. Consider extracting common input patterns into shared helper functions to improve maintainability.Example refactor:
+function enterBasicInputs() { + cy.get('[data-cy="sc-1-answer-option-2"]').click() + cy.get('[data-cy="mc-2-answer-option-2"]').click() + // ... common input logic +} + function enterValidPartialInputs() { - cy.get('[data-cy="sc-1-answer-option-2"]').click() - cy.get('[data-cy="mc-2-answer-option-2"]').click() + enterBasicInputs() // ... partial-specific logic }
1055-1095: Maintain consistent verification approach.The verification logic for partial inputs follows the same pattern as complete inputs. Apply the same helper function refactoring suggested for
verifyPersistentCompleteInputsto maintain consistency.
1113-1113: Remove empty comment line.The line contains only a comment marker with no content.
- //packages/shared-components/src/utils/completeSelectionResponse.ts (2)
1-7: Add JSDoc documentation to improve code clarity.Consider adding JSDoc documentation to explain the purpose of the function, its parameters, and return value. This will help other developers understand how to use this utility correctly.
+/** + * Completes a selection response by filling in missing values with -1 + * @param {Object} params - The parameters object + * @param {Record<number, number>} [params.existingResponse] - The existing response object + * @param {Record<number, number>} params.emptyResponses - The empty response template + * @returns {Record<number, number> | undefined} The completed response or undefined if no existing response + */ function completeSelectionResponse({
12-15: Enhance type safety and robustness of the response completion logic.Consider the following improvements:
- Use
Number(key)instead ofparseInt(key)for better performance and clarity.- Add type narrowing for the key parameter in forEach.
- Consider validating that the parsed numbers are valid indices.
const completedResponse = { ...emptyResponses } -Object.keys(existingResponse).forEach((key) => { - completedResponse[parseInt(key)] = existingResponse[parseInt(key)] ?? -1 +Object.keys(existingResponse).forEach((key: string) => { + const index = Number(key) + if (Number.isInteger(index) && index >= 0) { + completedResponse[index] = existingResponse[index] ?? -1 + } })
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
cypress/cypress/e2e/G-microlearning-workflow.cy.ts(4 hunks)packages/shared-components/src/hooks/useStudentResponse.ts(2 hunks)packages/shared-components/src/utils/completeSelectionResponse.ts(1 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
packages/shared-components/src/hooks/useStudentResponse.ts
[error] 34-34: Avoid the use of spread (...) syntax on accumulators.
Spread syntax should be avoided on accumulators (like those in .reduce) because it causes a time complexity of O(n^2).
Consider methods such as .splice or .push instead.
(lint/performance/noAccumulatingSpread)
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: SonarCloud
- GitHub Check: cypress-run
- GitHub Check: build
- GitHub Check: build
- GitHub Check: check
- GitHub Check: Analyze (javascript)
- GitHub Check: build
🔇 Additional comments (9)
packages/shared-components/src/hooks/useStudentResponse.ts (1)
1-8: LGTM! Clean import organization.The imports are well-structured, combining related imports and adding new dependencies appropriately.
cypress/cypress/e2e/G-microlearning-workflow.cy.ts (5)
15-16: LGTM! Constants improve test maintainability.The addition of
NRAnswerandFTAnswerconstants helps maintain consistency and makes it easier to update test values.
Line range hint
910-936: Well-structured function for handling complete inputs.The function systematically handles all question types and includes appropriate validation steps. The use of constants and clear sequencing improves maintainability.
Line range hint
980-1034: Comprehensive test coverage with edge cases.The test case effectively:
- Validates complete responses
- Verifies persistence across reloads
- Tests invalid inputs
- Handles async operations appropriately
1097-1114: Well-structured test for partial responses.The test case effectively validates partial response functionality with appropriate waits and verifications.
Line range hint
1115-1220: Thorough cleanup with verification.The cleanup process systematically removes all test data and includes verification steps to ensure proper deletion.
packages/shared-components/src/utils/completeSelectionResponse.ts (3)
8-10: LGTM! Clean early return pattern.The early return pattern is well implemented, making the code flow clear and maintainable.
17-20: LGTM! Clean export pattern.The return statement and default export are appropriately implemented.
1-20: Verify consistent usage of -1 as the default value across the codebase.Let's ensure that the -1 default value for incomplete selections is consistently used across all related components.
✅ Verification successful
Consistent usage of -1 as default value confirmed
The codebase consistently uses -1 as the marker for invalid/skipped selections, with proper filtering in place across all selection-related components.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any inconsistent default values in selection-related files rg -g '*.{ts,tsx}' '(default|incomplete|skipped|invalid).*selection.*=.*[0-9]+' --no-filename # Search for direct assignments of -1 in selection-related files rg -g '*.{ts,tsx}' 'selection.*=.*-1' --no-filenameLength of output: 468
Script:
#!/bin/bash # Search for selection-related function implementations ast-grep --pattern 'function $FUNC($$$) { $$$ selection $$$ }' # Search for selection-related type definitions rg -g '*.{ts,tsx}' 'type.*Selection|interface.*Selection' # Search for any numeric constants related to selections rg -g '*.{ts,tsx}' 'const.*selection.*=.*[0-9]|SELECTION.*=.*[0-9]' -iLength of output: 72129
|
sjschlapbach
changed the title
Summary by CodeRabbit
New Features
-1value.Bug Fixes
Refactor
undefinedvalues from selection responses.