chore: unify and simplify types structure for element options #4475
Conversation
… package definitions
Current Aviator status
This PR was merged manually (without Aviator). Merging manually can negatively impact the performance of the queue. Consider using Aviator next time.
See the real-time status of this PR on the
Aviator webapp.
Use the Aviator Chrome Extension
to see the status of your PR within GitHub.
|
dosubot
bot
added
the
size:L
📝 WalkthroughWalkthroughThe pull request introduces comprehensive type modifications across multiple packages, focusing on enhancing type definitions for question elements in the GraphQL schema. The changes primarily involve updating type imports from Changes
Possibly related PRs
✨ 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 (
|
dosubot
bot
added
size:XXL
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
packages/graphql/src/lib/validateNumericalOptions.ts (1)
Line range hint
3-31: Validate all solution ranges or exact solutions instead of just the first.
You currently check only the first item in the lists. If multiple solution ranges or exact solutions can exist, users might provide partially valid data. Consider iterating through all elements to ensure full validation.- const invalidSolutionRange = - options.solutionRanges && - (options.solutionRanges.length === 0 || - ((options.solutionRanges[0]?.min === null || - typeof options.solutionRanges[0]?.min === 'undefined') && - (options.solutionRanges[0]?.max === null || - typeof options.solutionRanges[0]?.max === 'undefined'))) + const hasInvalidSolutionRange = + options.solutionRanges?.some( + (range) => + (range.min === null || typeof range.min === 'undefined') && + (range.max === null || typeof range.max === 'undefined') + ) + const invalidSolutionRange = + !options.solutionRanges || options.solutionRanges.length === 0 || hasInvalidSolutionRange
🧹 Nitpick comments (8)
packages/graphql/src/lib/validateKPRIMOptions.ts (1)
1-1: Consider using a more descriptive error handling strategy instead ofconsole.error.
Currently, this function returns a boolean and logs a string toconsole.error. For better maintainability (e.g., i18n, structured logs, consistency across your application), consider throwing a well-typed error or returning an object containing an error message.packages/graphql/src/lib/validateSCOptions.ts (1)
1-1: Use consistent error reporting.
At the moment, you're usingconsole.errorfor reporting validation failures, just like in other validation functions. If your codebase adopts a consistent error-handling or logging pattern, consider abstracting error logging, or switching to exceptions for uniformity.packages/graphql/src/lib/validateMCOptions.ts (1)
1-1: Maintain a uniform logging or error strategy across validation functions.
Similar tovalidateSCOptionsandvalidateKPRIMOptions, you log errors withconsole.error. To maintain consistency and possibly integrate advanced logging in the future, structure your errors in a common format.packages/graphql/src/lib/validateNumericalOptions.ts (1)
1-1: Consider aligning with the shared-choices approach if feasible.
For code consistency, you might unify all validation flows (including numerical) under a shared abstraction. Even though numerical questions differ from choice-based questions, a shared approach or partial code reuse can reduce duplication.packages/graphql/src/lib/validateSharedChoicesFields.ts (1)
4-4: Confirm no redundant validations.
The retrieved learnings mention thatvalidateSharedChoicesFieldsis comprehensive for SC, MC, and KPRIM question types. Ensure other specialized validation functions (validateSCOptions,validateMCOptions, etc.) do not replicate these checks unnecessarily.packages/graphql/src/lib/validateAndProcessElementOptions.ts (1)
10-10: Consider removing or repurposing thefeedbackfield.
It's marked as// unused. If it truly has no purpose, removing it helps keep the interface minimal and easier to maintain.export interface ElementOptionsArgs { ... - feedback?: string | null // unused ... }packages/graphql/src/lib/validateSelectionOptions.ts (1)
Line range hint
3-31: Enhance error handling by throwing errors instead of logging.Right now, this validation function logs errors using
console.errorand returns a boolean. While this is functional, throwing typed errors or returning structured responses can make error handling more robust and traceable across the system. Consider refactoring to use thrown errors or a standardized error format to improve testability and clarity.Here’s an example snippet:
-function validateSelectionOptions(options?: ElementOptionsArgs | null) { +function validateSelectionOptions(options?: ElementOptionsArgs | null): true { // ... if (!options || typeof options.hasSampleSolution !== 'boolean' || options.hasSampleSolution === null) { - console.error('Options and sample solution flag are required for selection questions') - return false + throw new Error('Options and sample solution flag are required for selection questions') } // ... return true }packages/types/src/index.ts (1)
199-244: Excellent architectural improvements to the type system.The changes demonstrate a systematic approach to type definition with several key improvements:
- Consistent separation of restrictions into dedicated types
- Unified nullability handling across all options
- Clear inheritance hierarchy with
BaseElementOptions- Standardized naming convention using 'Element' instead of 'Question'
These changes will improve maintainability and type safety across the codebase.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (3)
packages/graphql/src/ops.schema.jsonis excluded by!**/**/ops.schema.jsonpackages/graphql/src/ops.tsis excluded by!**/**/ops.tspackages/graphql/src/public/schema.graphqlis excluded by!**/**/public/**
📒 Files selected for processing (22)
apps/func-response-processor/src/index.ts(1 hunks)packages/graphql/src/lib/validateAndProcessElementOptions.ts(2 hunks)packages/graphql/src/lib/validateElementInputs.ts(2 hunks)packages/graphql/src/lib/validateFreeTextOptions.ts(1 hunks)packages/graphql/src/lib/validateKPRIMOptions.ts(1 hunks)packages/graphql/src/lib/validateMCOptions.ts(1 hunks)packages/graphql/src/lib/validateNumericalOptions.ts(1 hunks)packages/graphql/src/lib/validateSCOptions.ts(1 hunks)packages/graphql/src/lib/validateSelectionOptions.ts(1 hunks)packages/graphql/src/lib/validateSharedChoicesFields.ts(1 hunks)packages/graphql/src/schema/elementData.ts(10 hunks)packages/graphql/src/schema/question.ts(3 hunks)packages/shared-components/src/ChoicesQuestion.tsx(2 hunks)packages/shared-components/src/FreeTextQuestion.tsx(2 hunks)packages/shared-components/src/NumericalQuestion.tsx(2 hunks)packages/shared-components/src/SelectionQuestion.tsx(2 hunks)packages/shared-components/src/evaluation/FTEvaluation.tsx(1 hunks)packages/shared-components/src/evaluation/NREvaluation.tsx(1 hunks)packages/shared-components/src/evaluation/SEEvaluation.tsx(2 hunks)packages/shared-components/src/questions/SELECTIONAnswerOptions.tsx(2 hunks)packages/shared-components/src/utils/validateResponse.ts(3 hunks)packages/types/src/index.ts(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- apps/func-response-processor/src/index.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/graphql/src/schema/question.ts
- packages/graphql/src/schema/elementData.ts
🧰 Additional context used
📓 Learnings (3)
packages/graphql/src/lib/validateKPRIMOptions.ts (2)
Learnt from: sjschlapbach
PR: uzh-bf/klicker-uzh#4468
File: packages/graphql/src/lib/validateKPRIMOptions.ts:4-15
Timestamp: 2025-01-17T17:24:24.190Z
Learning: The validateSharedChoicesFields function performs comprehensive validation for choice-based questions (SC, MC, KPRIM), including:
1. Checking if options and options.choices are defined
2. Ensuring at least one choice exists
3. Validating each choice has valid ix and value
4. Validating displayMode
5. Validating sample solution and feedback flags
6. Validating correct flags when sample solution is enabled
7. Validating feedback when both flags are enabled
Therefore, additional null/undefined checks in the specific validation functions (validateSCOptions, validateMCOptions, validateKPRIMOptions) are redundant.
Learnt from: sjschlapbach
PR: uzh-bf/klicker-uzh#4468
File: packages/graphql/src/lib/validateKPRIMOptions.ts:4-15
Timestamp: 2025-01-17T17:24:24.190Z
Learning: In validateKPRIMOptions and other choice-based validation functions (SC, MC), there's no need to check for null/undefined options and choices as these are already validated by validateSharedChoicesFields which is called at the beginning of these functions.
packages/graphql/src/lib/validateSCOptions.ts (1)
Learnt from: sjschlapbach
PR: uzh-bf/klicker-uzh#4468
File: packages/graphql/src/lib/validateKPRIMOptions.ts:4-15
Timestamp: 2025-01-17T17:24:24.190Z
Learning: The validateSharedChoicesFields function performs comprehensive validation for choice-based questions (SC, MC, KPRIM), including:
1. Checking if options and options.choices are defined
2. Ensuring at least one choice exists
3. Validating each choice has valid ix and value
4. Validating displayMode
5. Validating sample solution and feedback flags
6. Validating correct flags when sample solution is enabled
7. Validating feedback when both flags are enabled
Therefore, additional null/undefined checks in the specific validation functions (validateSCOptions, validateMCOptions, validateKPRIMOptions) are redundant.
packages/graphql/src/lib/validateSharedChoicesFields.ts (1)
Learnt from: sjschlapbach
PR: uzh-bf/klicker-uzh#4468
File: packages/graphql/src/lib/validateKPRIMOptions.ts:4-15
Timestamp: 2025-01-17T17:24:24.190Z
Learning: The validateSharedChoicesFields function performs comprehensive validation for choice-based questions (SC, MC, KPRIM), including:
1. Checking if options and options.choices are defined
2. Ensuring at least one choice exists
3. Validating each choice has valid ix and value
4. Validating displayMode
5. Validating sample solution and feedback flags
6. Validating correct flags when sample solution is enabled
7. Validating feedback when both flags are enabled
Therefore, additional null/undefined checks in the specific validation functions (validateSCOptions, validateMCOptions, validateKPRIMOptions) are redundant.
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: SonarCloud
- GitHub Check: check
- GitHub Check: Analyze (javascript)
- GitHub Check: cypress-run
🔇 Additional comments (33)
packages/shared-components/src/evaluation/NREvaluation.tsx (2)
3-3: Confirmed alignment with the new type import.
It's good to see the import ofNumericalElementOptionsandNumericalInstanceEvaluationhere, aligning this component with the broader type refactor. No issues found with the import statement.
11-11: Confirm consistency with existing references.
ChangingoptionstoNumericalElementOptionsis consistent with the PR objectives. However, ensure that any external references or consuming components expectingNumericalQuestionOptionsare updated accordingly.packages/shared-components/src/NumericalQuestion.tsx (2)
2-2: Validate import usage across all referencing components.
ImportingNumericalElementOptionsandNumericalInstanceEvaluationaligns with the updated structure. Confirm all references to these types in the codebase are synchronized to avoid mismatches.
16-16: Ensure typed properties match user input validations.
Switching fromNumericalQuestionOptionstoNumericalElementOptionsis aligned with the new design. Double-check that all property references (e.g.,restrictions,accuracy) are valid and that related validation logic (likevalidateNumericalResponse) still behaves correctly with the new type.packages/shared-components/src/utils/validateResponse.ts (2)
2-3: Imports match the new element option types. Good job!
These updated import statements correctly align with the unified element options structure.
70-70: Ensure consistent usage of new FreeTextElementOptions throughout the codebase.
The type update foroptionslooks correct here. Verify that any functions or components referencingFreeTextQuestionOptionshave also been updated to avoid type mismatches.packages/graphql/src/lib/validateKPRIMOptions.ts (1)
Line range hint
4-10: Validate the non-null assertions.
BecausevalidateSharedChoicesFields(options)ensuresoptionsandoptions.choicesare valid before returningtrue, your use ofoptions!.choices!is logically sound. However, if you anticipate future changes tovalidateSharedChoicesFields, consider explicitly handling null or undefined in a single line with a check or a type guard, maintaining clarity over the entire codebase.packages/graphql/src/lib/validateSCOptions.ts (1)
Line range hint
4-16: Ensure test coverage for sample solution logic.
This function checks for a single correct answer when sample solution is enabled. Ensure that your tests cover boundary cases (e.g., no choices marked correct, multiple choices marked correct, etc.).packages/graphql/src/lib/validateMCOptions.ts (1)
Line range hint
4-16: Add tests for edge cases.
For MC questions with sample solutions, test scenarios with zero correct answers, exactly one correct answer, and multiple correct answers to ensure the validation logic works comprehensively.packages/graphql/src/lib/validateSharedChoicesFields.ts (1)
2-2: Renamed import to reflect the new element-centric approach.
Great that you're transitioning fromQuestionOptionsArgstoElementOptionsArgs. Be sure to check the rest of the codebase so that any references to the old import name remain consistent and error-free.packages/graphql/src/lib/validateElementInputs.ts (2)
2-2: Maintain consistency with the new type.
You've replacedQuestionOptionsArgswithElementOptionsArgsin the import. Confirm that all references within and beyond this file have been updated and are in sync with the new type definition.
11-11: Interface property update is aligned with the new element-based structure.
This is a purposeful rename fromQuestionOptionsArgstoElementOptionsArgs. Good job on updating the interface property as well. No issues detected with this line.packages/graphql/src/lib/validateAndProcessElementOptions.ts (1)
43-43: Good alignment of the function parameter with the new interface.
SwitchingoptionstoElementOptionsArgsmaintains consistency across all element types. Verify that any downstream usage expects the updated structure.packages/graphql/src/lib/validateFreeTextOptions.ts (2)
1-1: Import new unified type.
This import aligns with the new approach to unify element option types across the codebase.
3-3: Ensure consistent usage and validation coverage.
Converting the function argument fromQuestionOptionsArgstoElementOptionsArgsis consistent with the broader refactor. Double-check that all calls and unit tests useElementOptionsArgscorrectly to avoid any potential type mismatch.packages/shared-components/src/evaluation/FTEvaluation.tsx (2)
2-2: Refined import statement.
ReplacingFreeTextQuestionOptionswithFreeTextElementOptionsimport ensures consistency with the updated type structure.
9-9: Interface property type updated.
SwitchingoptionstoFreeTextElementOptionsmatches the newly unified type definitions. Confirm downstream consumers also use the updated type without conflicts.packages/shared-components/src/FreeTextQuestion.tsx (2)
2-2: Imports aligned with new definitions.
Good job favoringFreeTextElementOptionsto keep the codebase consistent with the new naming convention.
16-16: Updated property signature.
SwitchingoptionsfromFreeTextQuestionOptionstoFreeTextElementOptionsproperly reflects the refactor. Ensure that tests or any related QA steps validate this new shape thoroughly.packages/shared-components/src/SelectionQuestion.tsx (2)
2-2: New import for selection element.
The import ofSelectionElementOptionsaligns with the broader replacement of older selection types, improving maintainability.
18-18: Revised props to match unified selection options.
ChangingoptionstoSelectionElementOptionsis consistent with the updated structure. Verify that logic tied to the old type, such as defaults or validations, behaves correctly.packages/shared-components/src/questions/SELECTIONAnswerOptions.tsx (2)
11-11: Ensured consistency inSELECTIONAnswerOptionsProps.Switching from
SelectionQuestionOptionstoSelectionElementOptionsmaintains a clear naming convention and aligns with the broader refactor.
1-1: Confirmed import alignment with updated type naming.Great job updating the import to
SelectionElementOptions. Ensure all downstream dependencies also use the new type.✅ Verification successful
Import alignment verified across all dependencies
The type
SelectionElementOptionsis consistently used and correctly imported from '@klicker-uzh/graphql/dist/ops' across all components and GraphQL operations in the codebase.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify all references to SelectionElementOptions across the codebase rg -A 3 "SelectionElementOptions"Length of output: 68667
packages/shared-components/src/evaluation/SEEvaluation.tsx (2)
2-2: Import statement aligns with the refactored type.Good consistency in updating the import to
SelectionElementOptions.
17-17: Parameter signature correctly switched toSelectionElementOptions.The updated parameter type reflects the new structure for selection-based questions. This appears correct and consistent with the rest of the code.
packages/shared-components/src/ChoicesQuestion.tsx (2)
2-2: Import updated toChoiceElementOptions.The import name now matches the unified element option types. Looks good.
25-25: Interface property updated to modern type definition.Replacing
ChoiceQuestionOptionswithChoiceElementOptionsensures consistency across choice-based elements.packages/graphql/src/lib/validateSelectionOptions.ts (1)
1-1: Confirm consistency of the newElementOptionsArgsimport.It's good to see that you're importing
ElementOptionsArgsfrom the centralized location. Please confirm that the rest of the codebase no longer references the oldQuestionOptionsArgs.As a follow-up, run this script to locate any remaining references to
QuestionOptionsArgs:✅ Verification successful
Migration to
ElementOptionsArgsis complete and consistentThe codebase shows no remaining references to
QuestionOptionsArgs, andElementOptionsArgsis consistently used across all validation files.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash rg --fixed-strings "QuestionOptionsArgs" -A 5Length of output: 45
Script:
#!/bin/bash # Verify file exists and repository is properly cloned echo "=== Checking file existence ===" ls packages/graphql/src/lib/validateSelectionOptions.ts echo -e "\n=== Searching for both type names ===" echo "ElementOptionsArgs references:" rg "ElementOptionsArgs" echo -e "\nQuestionOptionsArgs references:" rg "QuestionOptionsArgs" echo -e "\n=== Using ast-grep for type references ===" echo "ElementOptionsArgs type references:" ast-grep --pattern 'type $_: ElementOptionsArgs' ast-grep --pattern 'interface $_ extends ElementOptionsArgs'Length of output: 2910
packages/types/src/index.ts (5)
199-202: Well-structured type definition for numerical restrictions!The
NumericalRestrictionstype clearly defines optional validation ranges with explicit null handling.
209-216: Excellent use of inheritance pattern!The
BaseElementOptionsinterface effectively shares common properties across different element types, promoting code reuse and maintainability.
228-235: Consistent type structure for free text options!The separation of
FreeTextRestrictionsand its integration intoElementOptionsFreeTextfollows a clean and consistent pattern.
237-244: Enhanced type safety with explicit entry type!The introduction of
SelectionAnswerCollectionEntryimproves code clarity and maintainability by providing a reusable, well-defined structure for answer collection entries.
219-226: Verify the impact of nullable properties.The interface now allows null values for multiple properties. While this provides more flexibility, it requires careful handling in the consuming code.
Let's check for potential null-safety issues:
✅ Verification successful
Nullable properties are safely handled throughout the codebase ✅
The codebase consistently uses null coalescing (
??) and optional chaining (?.) operators when accessing these properties. All consuming code properly handles the nullable values, with appropriate fallbacks toundefinedwhere necessary.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for direct property access without null checks rg -t typescript "options\.(accuracy|placeholder|restrictions|solutionRanges|exactSolutions)\." --glob '!packages/types/**' # Search for potential type assertions or non-null assertions rg -t typescript "options\.(accuracy|placeholder|restrictions|solutionRanges|exactSolutions)!" --glob '!packages/types/**'Length of output: 320
Script:
#!/bin/bash # Search for property access patterns including destructuring rg "\b(options\.(accuracy|placeholder|restrictions|solutionRanges|exactSolutions)|accuracy|placeholder|restrictions|solutionRanges|exactSolutions)\s*[:=]" --type-add 'ts:*.{ts,tsx}' -t ts --glob '!packages/types/**' -A 2 # Search for non-null assertions rg "\b(options\.(accuracy|placeholder|restrictions|solutionRanges|exactSolutions)|accuracy|placeholder|restrictions|solutionRanges|exactSolutions)!" --type-add 'ts:*.{ts,tsx}' -t ts --glob '!packages/types/**' # Search for type guards or null checks rg "\b(options\.(accuracy|placeholder|restrictions|solutionRanges|exactSolutions))(\s*\?\s*\.|&&|\|\||\?\.)" --type-add 'ts:*.{ts,tsx}' -t ts --glob '!packages/types/**'Length of output: 52816
|
klicker-uzh
|
||||||||||||||||||||||||||||
| Project |
klicker-uzh
|
| Branch Review |
unify-element-types
|
| Run status |
|
| Run duration | 18m 41s |
| Commit |
|
| Committer | Julius Schlapbach |
| View all properties for this run ↗︎ | |
| Test results | |
|---|---|
Failures
|
0
|
Flaky
|
0
|
Pending
|
0
|
Skipped
|
0
|
Passing
|
311
|
| View all changes introduced in this branch ↗︎ | |
Summary by CodeRabbit