fix: summary calculation in multi choice questions#4022
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎ 2 Skipped Deployments
|
|
Thank you for following the naming conventions for pull request titles! 🙏 |
WalkthroughThe changes in this pull request focus on enhancing the logic for calculating survey summaries within the Changes
Possibly related PRs
Suggested labels
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]](proxy.php?url=https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- apps/web/app/(app)/environments/[environmentId]/surveys/[surveyId]/(analysis)/summary/lib/surveySummary.ts (4 hunks)
| }, {}); | ||
|
|
||
| const otherValues: TSurveyQuestionSummaryMultipleChoice["choices"][number]["others"] = []; | ||
| let totalResponseCount = 0; |
There was a problem hiding this comment.
Fix incorrect totalResponseCount calculation in multiple-choice summaries
The totalResponseCount is incremented inside the loop over each selected value. This means that if a respondent selects multiple options, totalResponseCount is incremented multiple times for a single response, leading to inflated counts and incorrect percentage calculations.
To correct this, increment totalResponseCount once per response that has at least one valid answer, regardless of how many options are selected.
Apply the following changes:
let totalResponseCount = 0;
responses.forEach((response) => {
const responseLanguageCode = getLanguageCode(survey.languages, response.language);
const answer =
responseLanguageCode === "default"
? response.data[question.id]
: checkForI18n(response, question.id, survey, responseLanguageCode);
+ let hasValidAnswer = false;
if (Array.isArray(answer)) {
answer.forEach((value) => {
if (value) {
if (questionChoices.includes(value)) {
choiceCountMap[value]++;
} else if (isOthersEnabled) {
otherValues.push({
value,
person: response.person,
personAttributes: response.personAttributes,
});
}
+ hasValidAnswer = true;
}
});
} else if (typeof answer === "string") {
if (answer) {
if (questionChoices.includes(answer)) {
choiceCountMap[answer]++;
} else if (isOthersEnabled) {
otherValues.push({
value: answer,
person: response.person,
personAttributes: response.personAttributes,
});
}
+ hasValidAnswer = true;
}
}
+ if (hasValidAnswer) {
+ totalResponseCount++;
+ }
});This ensures totalResponseCount accurately reflects the number of respondents who answered the question.
Also applies to: 353-366, 368-369, 386-387, 395-398, 405-405
There was a problem hiding this comment.
@gupta-piyush19 is this what you meant? what about the suggestion?
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
| } else if (isOthersEnabled) { | ||
| otherValues.push({ | ||
| value, | ||
| value: answer, |
There was a problem hiding this comment.
Complete the missing otherValues.push call
At line 373, within the else if (isOthersEnabled) block for single-choice answers, the object with value: answer is not being added to the otherValues array. This omission means that responses selecting "Other" are not recorded properly.
To fix this, wrap the object in an otherValues.push({ ... }) call:
} else if (isOthersEnabled) {
- value: answer,
+ otherValues.push({
+ value: answer,
+ person: response.person,
+ personAttributes: response.personAttributes,
+ });
}This change ensures that "Other" responses are captured and included in the summary.
📝 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.
| value: answer, | |
| otherValues.push({ | |
| value: answer, | |
| person: response.person, | |
| personAttributes: response.personAttributes, | |
| }); |
jobenjada
left a comment
jobenjada
left a comment
There was a problem hiding this comment.
pls merge after changes discussed :)
| }, {}); | ||
|
|
||
| const otherValues: TSurveyQuestionSummaryMultipleChoice["choices"][number]["others"] = []; | ||
| let totalResponseCount = 0; |
There was a problem hiding this comment.
@gupta-piyush19 is this what you meant? what about the suggestion?
…fix/survey-summary-multi-choice-calculation
|
#4024 fixing here by accident |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (2)
apps/web/app/(app)/environments/[environmentId]/surveys/[surveyId]/(analysis)/summary/components/SummaryDropOffs.tsx (1)
11-14: Consider using design system tokens for icon alignment.The
mt-[3px]appears to be a magic number for vertical alignment. Consider using your design system's spacing tokens or flexbox alignment properties for more maintainable styling.- return <Icon className="mt-[3px] h-5 w-5 shrink-0 text-slate-600" />; + return <Icon className="h-5 w-5 shrink-0 text-slate-600" />;Then, handle the alignment in the parent flex container:
- <div className="col-span-3 flex gap-3 pl-4 md:pl-6"> + <div className="col-span-3 flex items-center gap-3 pl-4 md:pl-6">apps/web/app/(app)/environments/[environmentId]/surveys/[surveyId]/(analysis)/summary/components/MultipleChoiceSummary.tsx (1)
Line range hint
1-164: Well-structured enhancement to multi-choice summary display.The changes effectively improve the clarity of multiple-choice question summaries by:
- Adding a clear indicator of total selections for multiple-choice questions
- Using more precise terminology throughout the component
- Maintaining existing functionality while enhancing the user experience
These modifications align well with the PR's objective of fixing summary calculations in multi-choice questions.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (6)
- apps/web/app/(app)/environments/[environmentId]/surveys/[surveyId]/(analysis)/summary/components/MultipleChoiceSummary.tsx (3 hunks)
- apps/web/app/(app)/environments/[environmentId]/surveys/[surveyId]/(analysis)/summary/components/SummaryDropOffs.tsx (2 hunks)
- apps/web/app/(app)/environments/[environmentId]/surveys/[surveyId]/(analysis)/summary/lib/surveySummary.ts (4 hunks)
- packages/lib/response/tests/mocks/data.mock.ts (1 hunks)
- packages/lib/utils/questions.tsx (1 hunks)
- packages/types/surveys/types.ts (2 hunks)
🔇 Additional comments (14)
apps/web/app/(app)/environments/[environmentId]/surveys/[surveyId]/(analysis)/summary/components/SummaryDropOffs.tsx (3)
2-3: LGTM: Import statements are properly structured.The new imports correctly bring in the required icon utility and type definitions.
40-42: LGTM: Clean implementation of icon integration.The flex layout properly accommodates the new icon while maintaining responsive behavior and consistent spacing.
40-43: Verify integration with updated summary calculation.While the display changes look good, please ensure that the
questionTypebeing passed through thedropOffarray correctly reflects the updated summary calculation logic for multi-choice questions mentioned in the PR description.✅ Verification successful
Based on the code analysis, I can now provide a definitive response about the integration with the updated summary calculation.
The display changes are correctly integrated with the summary calculation.
The code shows that:
- The
getSurveySummaryDropOfffunction insurveySummary.tscorrectly calculates and includes thequestionTypein the dropOff array.- The
SummaryDropOffscomponent receives and displays this data without any modifications.- The
questionTypefield is properly typed in theTSurveySummaryinterface, ensuring type safety.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for related summary calculation logic rg -t typescript "getSurveySummary|getQuestionSummary" --glob "!node_modules" # Look for questionType usage in survey summary types rg -t typescript "questionType.*TSurveySummary" --glob "!node_modules"Length of output: 222
Script:
#!/bin/bash # Search for related summary calculation logic without typescript filter rg "getSurveySummary|getQuestionSummary" # Look for survey summary type definitions and usage rg "type.*SurveySummary|interface.*SurveySummary" # Look for dropOff calculation or processing rg "dropOff.*question.*type|question.*type.*dropOff" -A 3Length of output: 3383
Script:
#!/bin/bash # Check the implementation of getSurveySummaryDropOff and related types cat "apps/web/app/(app)/environments/[environmentId]/surveys/[surveyId]/(analysis)/summary/lib/surveySummary.ts" # Check the survey summary type definition cat "packages/types/surveys/types.ts" # Look for dropOff related components and their usage rg -l "SummaryDropOffs"Length of output: 120086
apps/web/app/(app)/environments/[environmentId]/surveys/[surveyId]/(analysis)/summary/components/MultipleChoiceSummary.tsx (2)
72-79: LGTM! Clear and consistent UI enhancement.The addition of selection count information for multiple-choice questions improves the UI by providing better visibility into response data. The implementation follows good practices with:
- Clean conditional rendering
- Consistent UI pattern using icon + text in a badge
- Type-safe implementation
109-109: LGTM! Improved terminology accuracy.The change from "response" to "selection" is more precise for multiple-choice context and properly handles singular/plural cases.
packages/lib/response/tests/__mocks__/data.mock.ts (2)
Line range hint
13-296: LGTM! Mock responses provide comprehensive test coverage.The mock response data structure is well-designed with good coverage for:
- Different response states (finished/unfinished)
- Various attribute combinations
- Different tag combinations
- Multiple choice question responses
This will help ensure robust testing of the summary calculation fix.
Line range hint
369-374: Verify test coverage for multi-choice summary calculation.Since this PR fixes summary calculation for multi-choice questions, let's verify if there are sufficient test cases covering both single and multiple choice scenarios.
✅ Verification successful
Test coverage for multi-choice questions is sufficient.
Looking at the test file and mock data, there is comprehensive test coverage for both single and multiple choice questions:
- The mock survey data includes both types:
MultipleChoiceSinglequestion with ID "hagrboqlnynmxh3obl1wvmtl"MultipleChoiceMultiquestion with ID "uvy0fa96e1xpd10nrj1je662"- The mock responses data includes answers for both question types
- The test suite includes specific tests for survey summary calculations that verify the handling of these responses
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for test files and coverage related to survey summary calculation # Look for test files related to survey summary echo "Looking for survey summary test files..." fd -e test.ts -e spec.ts | rg -i "survey.*summary" # Look for test cases covering multiple choice questions echo "Looking for multiple choice test cases..." rg -A 5 "test\(.*multiple.*choice"Length of output: 273
Script:
#!/bin/bash # Let's try a broader search to find relevant test files and test cases # First, find all test files in the response directory echo "Test files in response directory:" fd -e test.ts -e spec.ts . packages/lib/response/ # Look for question type related tests echo -e "\nTest cases for question types:" rg -l "questionType.*multiple" packages/lib/response/ # Check the specific mock data file content echo -e "\nMock data content:" cat packages/lib/response/tests/__mocks__/data.mock.ts # Look for test descriptions related to summary calculations echo -e "\nTest cases mentioning summary calculations:" rg "describe\(.*summary" -A 5 packages/lib/response/Length of output: 14623
Script:
#!/bin/bash # Let's look for the actual test file and its content since we know it exists echo "Content of response test file:" cat packages/lib/response/tests/response.test.ts # Also search for any test cases related to survey calculations echo -e "\nTest cases for calculations:" rg "test\(.*calculation|test\(.*summary" -A 5 packages/lib/response/tests/Length of output: 24959
apps/web/app/(app)/environments/[environmentId]/surveys/[surveyId]/(analysis)/summary/lib/surveySummary.ts (5)
238-238: LGTM: Adding questionType to dropOff arrayThe addition of
questionTypeto the dropOff array improves the data structure by providing more context about each question.
343-344: LGTM: Improved response counting logic for array responsesThe new implementation correctly separates
totalSelectionCount(total number of options selected) fromtotalResponseCount(number of respondents who answered). This fixes the previous issue where response counts were inflated for multiple-choice questions when a single respondent selected multiple options.Also applies to: 353-369
371-389: LGTM: Consistent handling of string responsesThe logic for handling single-choice responses (string type) now mirrors the array response handling, maintaining consistency in how responses are counted and ensuring accurate statistics.
396-398: LGTM: Accurate percentage calculationsThe percentage calculations now correctly use
totalSelectionCountas the denominator, which provides accurate representation of how frequently each option was chosen relative to all selections made.Also applies to: 405-408
415-416: LGTM: Clear distinction between response and selection countsThe summary now includes both
responseCount(number of respondents) andselectionCount(total options selected), providing clearer insights into user behavior in multiple-choice questions.packages/types/surveys/types.ts (2)
2109-2109: LGTM! Valuable addition for multiple-choice analytics.The
selectionCountproperty enhances the analytics by tracking the total number of selections made across all choices in multiple-choice questions, which is particularly useful for questions allowing multiple selections.
2428-2428: LGTM! Improves drop-off analysis capabilities.The
questionTypeproperty in the drop-off data provides valuable context about which types of questions are causing users to abandon the survey, enabling better UX optimization.
| export const getQuestionIcon = (type: TSurveyQuestionTypeEnum) => { | ||
| return questionTypes.find((questionType) => questionType.id === type)?.icon; | ||
| }; |
There was a problem hiding this comment.
Consider using existing QUESTIONS_ICON_MAP instead of creating a new function.
The new getQuestionIcon function appears to be redundant as there's already a pre-computed QUESTIONS_ICON_MAP that serves the same purpose. The current implementation has several drawbacks:
- Performance: Uses
Array.find()on each call instead of constant-time object lookup - Consistency: Returns raw icon component while
QUESTIONS_ICON_MAPreturns styled JSX elements - Type safety: May return undefined while
QUESTIONS_ICON_MAPguarantees a value for each enum
If you need a separate function, consider this implementation for better performance and consistency:
-export const getQuestionIcon = (type: TSurveyQuestionTypeEnum) => {
- return questionTypes.find((questionType) => questionType.id === type)?.icon;
-};
+export const getQuestionIcon = (type: TSurveyQuestionTypeEnum) => {
+ return QUESTIONS_ICON_MAP[type];
+};Or if you specifically need the raw icon component:
-export const getQuestionIcon = (type: TSurveyQuestionTypeEnum) => {
- return questionTypes.find((questionType) => questionType.id === type)?.icon;
-};
+export const getQuestionIcon = (type: TSurveyQuestionTypeEnum) => {
+ const questionType = questionTypes.find((qt) => qt.id === type);
+ if (!questionType) {
+ throw new Error(`Question type ${type} not found`);
+ }
+ return questionType.icon;
+};📝 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 const getQuestionIcon = (type: TSurveyQuestionTypeEnum) => { | |
| return questionTypes.find((questionType) => questionType.id === type)?.icon; | |
| }; | |
| export const getQuestionIcon = (type: TSurveyQuestionTypeEnum) => { | |
| return QUESTIONS_ICON_MAP[type]; | |
| }; |
| dropOffCount: 0, | ||
| dropOffPercentage: 0, | ||
| headline: "Question Text", | ||
| questionType: "openText", |
There was a problem hiding this comment.
Fix inconsistency in mock question type.
The questionType is set to "openText" in the mock summary, but the corresponding survey questions are of type "MultipleChoiceSingle" and "MultipleChoiceMulti". This inconsistency could affect tests for the summary calculation fix.
Update the mock data to match the actual question type:
- questionType: "openText",
+ questionType: "MultipleChoiceSingle",📝 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.
| questionType: "openText", | |
| questionType: "MultipleChoiceSingle", |
gupta-piyush19
deleted the
fix/survey-summary-multi-choice-calculation
branch
2 participants
What does this PR do?
fixes summary calculation in multi choice questions
Fixes # (issue)
How should this be tested?
Checklist
Required
pnpm buildconsole.logsgit pull origin mainAppreciated
Summary by CodeRabbit
New Features
Bug Fixes
Style
sizeprop and modified visual presentation for "Last Used" labels across multiple components.