fix: inconsistencies in survey summary #4032
Conversation
…fix/survey-summary-multi-choice-calculation
…fix/survey-summary-multi-choice-calculation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎ |
WalkthroughThe changes in this pull request primarily involve modifications to various components related to survey management. Key updates include changes to caution messages, enhancements in decimal precision for percentage displays, and improvements in the handling of survey summaries and validations. The overall logic and control flow remain largely unchanged, focusing on cosmetic improvements and minor adjustments to enhance user experience and data accuracy. Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
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 (
|
|
Thank you for following the naming conventions for pull request titles! 🙏 |
![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
🧹 Outside diff range and nitpick comments (7)
apps/web/app/(app)/environments/[environmentId]/surveys/[surveyId]/(analysis)/summary/components/CalSummary.tsx (1)
45-45: Consider aligning variable naming with UI text.While the precision change is good, there's an inconsistency between the variable name (
skipped) and the UI text ("Dismissed"). This could cause confusion during maintenance.Consider applying this change for better clarity:
-questionSummary.skipped.percentage +questionSummary.dismissed.percentageNote: This change would need to be coordinated with the corresponding type definition in
TSurveyQuestionSummaryCal.apps/web/app/(app)/environments/[environmentId]/surveys/[surveyId]/(analysis)/summary/components/PictureChoiceSummary.tsx (1)
42-49: LGTM! Consider adding aria-label for accessibility.The additional info section for multi-select questions is well implemented. The visual indicator with InboxIcon provides good UX.
Consider adding an aria-label to improve accessibility:
- <div className="flex items-center rounded-lg bg-slate-100 p-2"> + <div className="flex items-center rounded-lg bg-slate-100 p-2" aria-label="Total selections count">apps/web/app/(app)/environments/[environmentId]/surveys/[surveyId]/(analysis)/summary/components/RatingSummary.tsx (2)
81-81: LGTM! Consider adding number formatting for consistency.The change to display percentages with 2 decimal places aligns well with other summary components and improves precision.
Consider using the
Intl.NumberFormatfor consistent number formatting across the application:-{convertFloatToNDecimal(result.percentage, 2)}% +{new Intl.NumberFormat('en-US', { minimumFractionDigits: 2, maximumFractionDigits: 2 }).format(result.percentage)}%
Line range hint
42-90: Consider performance optimizations.The component could benefit from some performance improvements:
- Memoize the click handler to prevent recreating it on each render:
const handleFilterClick = useCallback( (rating: number) => { setFilter( questionSummary.question.id, questionSummary.question.headline, questionSummary.question.type, "Is equal to", rating.toString() ); }, [questionSummary.question, setFilter] );
- Memoize the percentage calculations:
const choices = useMemo( () => questionSummary.choices.map((result) => ({ ...result, formattedPercentage: new Intl.NumberFormat('en-US', { minimumFractionDigits: 2, maximumFractionDigits: 2 }).format(result.percentage) })), [questionSummary.choices] );apps/web/app/(app)/environments/[environmentId]/surveys/[surveyId]/(analysis)/summary/components/OpenTextSummary.tsx (1)
105-115: LGTM! Consider making the width responsive.The changes improve readability by using
break-normaland setting a fixed width. However, consider using responsive width classes for better mobile support.-<TableCell width={180}> +<TableCell className="w-[120px] md:w-[180px]">apps/web/app/(app)/(survey-editor)/environments/[environmentId]/surveys/[surveyId]/edit/components/SurveyMenuBar.tsx (2)
64-64: LGTM! Consider making the warning message more specific.The new warning message is more action-oriented, but could be more specific about the consequences, e.g., "Changes may affect survey response data consistency."
- const cautionText = "Changes will lead to inconsistencies."; + const cautionText = "Changes may affect survey response data consistency.";
Line range hint
274-284: Consider enhancing the tooltip with more detailed guidance.The tooltip could provide more context about which types of changes might lead to inconsistencies and how to handle them safely.
<TooltipContent side={"top"} className="lg:hidden"> - <p className="py-2 text-center text-xs text-slate-500 dark:text-slate-400">{cautionText}</p> + <div className="max-w-xs py-2 text-center text-xs text-slate-500 dark:text-slate-400"> + <p>{cautionText}</p> + <p className="mt-1">Modifying questions or response options may impact data analysis. Consider creating a new survey version instead.</p> + </div> </TooltipContent>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (12)
- apps/web/app/(app)/(survey-editor)/environments/[environmentId]/surveys/[surveyId]/edit/components/SurveyMenuBar.tsx (1 hunks)
- apps/web/app/(app)/environments/[environmentId]/surveys/[surveyId]/(analysis)/summary/components/CTASummary.tsx (1 hunks)
- apps/web/app/(app)/environments/[environmentId]/surveys/[surveyId]/(analysis)/summary/components/CalSummary.tsx (2 hunks)
- apps/web/app/(app)/environments/[environmentId]/surveys/[surveyId]/(analysis)/summary/components/ConsentSummary.tsx (1 hunks)
- apps/web/app/(app)/environments/[environmentId]/surveys/[surveyId]/(analysis)/summary/components/MultipleChoiceSummary.tsx (2 hunks)
- apps/web/app/(app)/environments/[environmentId]/surveys/[surveyId]/(analysis)/summary/components/NPSSummary.tsx (1 hunks)
- apps/web/app/(app)/environments/[environmentId]/surveys/[surveyId]/(analysis)/summary/components/OpenTextSummary.tsx (2 hunks)
- apps/web/app/(app)/environments/[environmentId]/surveys/[surveyId]/(analysis)/summary/components/PictureChoiceSummary.tsx (3 hunks)
- apps/web/app/(app)/environments/[environmentId]/surveys/[surveyId]/(analysis)/summary/components/RankingSummary.tsx (1 hunks)
- apps/web/app/(app)/environments/[environmentId]/surveys/[surveyId]/(analysis)/summary/components/RatingSummary.tsx (1 hunks)
- apps/web/app/(app)/environments/[environmentId]/surveys/[surveyId]/(analysis)/summary/lib/surveySummary.ts (3 hunks)
- packages/types/surveys/types.ts (1 hunks)
✅ Files skipped from review due to trivial changes (1)
- apps/web/app/(app)/environments/[environmentId]/surveys/[surveyId]/(analysis)/summary/components/ConsentSummary.tsx
🔇 Additional comments (17)
apps/web/app/(app)/environments/[environmentId]/surveys/[surveyId]/(analysis)/summary/components/CTASummary.tsx (1)
47-47: Verify decimal precision consistency across all summary components.The change to display CTR with 2 decimal places looks good. Let's verify this is consistent across all summary components.
✅ Verification successful
Decimal precision is consistent across all summary components
The verification confirms that all summary components using
convertFloatToNDecimalare consistently using 2 decimal places for percentage and numerical values, including:
- CTASummary: CTR percentage
- CalSummary: Booked and skipped percentages
- PictureChoiceSummary: Result percentages
- RankingSummary: Average rankings
- NPSSummary: Group percentages
- MultipleChoiceSummary: Result percentages
- ConsentSummary: Summary percentages
- RatingSummary: Result percentages
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check decimal precision usage across all summary components # Expected: All summary components should use 2 decimal places # Search for convertFloatToNDecimal usage in summary components rg "convertFloatToNDecimal\([^,]+,\s*\d+" -A 1 "apps/web/app/(app)/environments/[^/]+/surveys/[^/]+/(analysis)/summary/components/*Summary.tsx"Length of output: 280
Script:
#!/bin/bash # Let's try a different approach to find summary components and their decimal precision usage # First, let's find all Summary.tsx files in the apps directory fd "Summary\.tsx$" apps/web # Then, let's search for convertFloatToNDecimal usage in these files rg "convertFloatToNDecimal\([^,]+,\s*\d+" apps/webLength of output: 3677
apps/web/app/(app)/environments/[environmentId]/surveys/[surveyId]/(analysis)/summary/components/RankingSummary.tsx (1)
41-41: LGTM! Decimal precision standardization.The change from 1 to 2 decimal places aligns with the broader effort to standardize numerical representations across survey summary components.
Let's verify the consistency across other summary components:
✅ Verification successful
Decimal precision standardization verified across all summary components
All summary components consistently use 2 decimal places with
convertFloatToNDecimal:
- RatingSummary:
convertFloatToNDecimal(result.percentage, 2)- RankingSummary:
convertFloatToNDecimal(result.avgRanking, 2)- PictureChoiceSummary:
convertFloatToNDecimal(result.percentage, 2)- NPSSummary:
convertFloatToNDecimal(questionSummary[group]?.percentage, 2)- MultipleChoiceSummary:
convertFloatToNDecimal(result.percentage, 2)- ConsentSummary:
convertFloatToNDecimal(summaryItem.percentage, 2)- CalSummary:
convertFloatToNDecimal(questionSummary.*.percentage, 2)- CTASummary:
convertFloatToNDecimal(questionSummary.ctr.percentage, 2)🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify decimal precision consistency across summary components # Expect: All summary components should use 2 decimal places # Search for convertFloatToNDecimal usage in summary components rg -A 1 "convertFloatToNDecimal" --glob "*Summary.tsx"Length of output: 6039
apps/web/app/(app)/environments/[environmentId]/surveys/[surveyId]/(analysis)/summary/components/CalSummary.tsx (1)
29-29: LGTM! Consistent with precision standardization.The increased decimal precision aligns with the standardization effort across summary components.
apps/web/app/(app)/environments/[environmentId]/surveys/[surveyId]/(analysis)/summary/components/PictureChoiceSummary.tsx (2)
78-78: LGTM! Consistent decimal precision.The change to display percentages with 2 decimal places aligns with other summary components, fixing the inconsistency as intended in the PR objectives.
83-83: LGTM! More accurate terminology.Good update to use "Selection/Selections" instead of "response/responses", which better reflects the action being counted.
apps/web/app/(app)/environments/[environmentId]/surveys/[surveyId]/(analysis)/summary/components/NPSSummary.tsx (1)
79-79: Verify consistent decimal precision across all survey components.The change to 2 decimal places aligns with the PR's goal of fixing inconsistencies.
Let's verify the consistency across other summary components:
✅ Verification successful
Decimal precision is consistent across all survey summary components
All survey summary components (NPSSummary, PictureChoiceSummary, MultipleChoiceSummary, ConsentSummary, CTASummary, CalSummary, and RatingSummary) consistently use
convertFloatToNDecimalwith 2 decimal places for percentage display. The change aligns perfectly with the existing codebase standards.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check decimal precision in all survey summary components # Expected: All components should use convertFloatToNDecimal with 2 decimal places rg -U "convertFloatToNDecimal.*percentage.*\d" --type tsxLength of output: 87
Script:
#!/bin/bash # Search for usage of convertFloatToNDecimal with percentage across all files rg -U "convertFloatToNDecimal.*percentage" # Also search for the function definition to understand its usage rg -U "function convertFloatToNDecimal|const convertFloatToNDecimal"Length of output: 1816
apps/web/app/(app)/environments/[environmentId]/surveys/[surveyId]/(analysis)/summary/components/OpenTextSummary.tsx (2)
Line range hint
117-122: LGTM! Consistent text wrapping behavior.The change maintains visual consistency between anonymous and identified users.
127-127: LGTM! Verify time formatting across all locales.The fixed width improves layout stability. However, let's verify the time formatting works well across different locales.
✅ Verification successful
Let me analyze the
timeSinceimplementation to verify the formatting.
Time formatting is properly implemented and consistent across the application
The implementation uses
date-fns'sformatDistancefunction which:
- Handles all locales correctly through its internal localization system
- Provides relative time formatting (e.g., "2 hours ago")
- Uses consistent formatting across all usages in the codebase
- The 120px width allocation is sufficient for the formatted output
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for all usages of timeSince to ensure consistent formatting rg "timeSince" -A 2Length of output: 18907
Script:
#!/bin/bash # Check the implementation of timeSince function cat packages/lib/time.tsLength of output: 3347
apps/web/app/(app)/environments/[environmentId]/surveys/[surveyId]/(analysis)/summary/components/MultipleChoiceSummary.tsx (4)
76-76: LGTM! Consistent with other summary components.The capitalization of "Selections" aligns with the standardization effort across survey summary components.
104-104: LGTM! Enhanced precision in percentage display.The change to display percentages with 2 decimal places improves consistency with other summary components and provides more precise data representation.
109-109: LGTM! Standardized selection count text.The conditional text formatting for singular/plural cases ("Selection"/"Selections") maintains proper grammar while adhering to the new capitalization standard.
Line range hint
76-109: Verify consistent formatting across all summary components.While the changes look good, let's verify that similar formatting is applied consistently across all survey summary components.
✅ Verification successful
Selection count and percentage formatting are consistent across summary components
Based on the search results, I can verify that:
- All summary components consistently use:
- "Selection(s)" with proper capitalization and pluralization
convertFloatToNDecimal(value, 2)for percentage formatting with 2 decimal placesThe formatting in
MultipleChoiceSummary.tsxmatches other summary components likePictureChoiceSummary.tsx,RatingSummary.tsx, etc., ensuring a consistent user experience across all question types.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for inconsistencies in selection count and percentage formatting across summary components # Expected: All summary components should use: # - Capitalized "Selection(s)" # - 2 decimal places for percentages # - Consistent conditional text formatting # Search for selection-related text formatting echo "Checking selection text formatting..." rg -i "selection[s]?" --type tsx # Search for percentage decimal formatting echo "Checking percentage decimal formatting..." rg "convertFloatToNDecimal" -A 1 --type tsxLength of output: 312
Script:
#!/bin/bash # Let's try without file type filtering since tsx is not recognized # Search for selection-related text formatting echo "Checking selection text formatting..." rg -i "selection[s]?" # Search for percentage decimal formatting echo "Checking percentage decimal formatting..." rg "convertFloatToNDecimal" -A 1Length of output: 35005
apps/web/app/(app)/environments/[environmentId]/surveys/[surveyId]/(analysis)/summary/lib/surveySummary.ts (4)
431-438: LGTM! Tracking total selections improves accuracy.The addition of
totalSelectionCountand its increment within the response loop is correct. This ensures accurate percentage calculations for multi-select picture questions.
450-451: LGTM! Percentage calculation now uses total selections.The percentage calculation now correctly uses
totalSelectionCountinstead oftotalResponseCount, which is more accurate for multi-select questions where each response can have multiple selections.
460-460: LGTM! Summary includes total selection count.Adding
selectionCountto the summary object provides valuable information about the total number of selections made across all responses.
Line range hint
431-460: Verify consistency with other multi-select question types.The changes improve accuracy for PictureSelection questions. Let's verify if other multi-select question types use the same approach.
✅ Verification successful
Based on the extensive search results, I can now verify the consistency of multi-select question types. The codebase shows that:
- The PictureSelection question type follows the same pattern as other multi-select questions (MultipleChoiceMulti) for handling selections and calculating percentages:
- Both use Array.isArray() to check responses
- Both track totalSelectionCount and totalResponseCount
- Both calculate percentages using the same formula with totalSelectionCount as denominator
- The logic handling is consistent across multi-select types:
- Same operators available (equals, doesNotEqual, includesAll, includesOne)
- Same array response handling
- Same validation rules
- The UI components handle both types similarly:
- Same filtering mechanisms
- Same summary display logic
- Same response processing
The implementation for PictureSelection questions is consistent with other multi-select question types, confirming the changes are properly aligned with the existing patterns.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for other question types that might need similar treatment ast-grep --pattern 'case TSurveyQuestionTypeEnum.$_: { $$$ totalSelectionCount $$$ }' # Search for percentage calculations in the file rg 'percentage.*totalSelectionCount' -A 2 -B 2Length of output: 142
Script:
#!/bin/bash # Let's try a different approach to find multi-select question handling rg "Array\.isArray.*answer" -B 5 -A 10 # Also search for totalSelectionCount usage rg "totalSelectionCount" -B 5 -A 5 # Look for question type enums to understand all possible types rg "TSurveyQuestionTypeEnum" -B 2 -A 10Length of output: 581908
packages/types/surveys/types.ts (1)
2139-2139: EnsureselectionCountis properly initialized and utilizedThe addition of
selectionCounttoZSurveyQuestionSummaryPictureSelectionenhances the data captured for picture selection questions. Please verify that all instances whereZSurveyQuestionSummaryPictureSelectionis created or manipulated are updated to include this new property to prevent potential runtime errors.Run the following script to find all usages of
ZSurveyQuestionSummaryPictureSelectionand check forselectionCounthandling:
| <div> | ||
| <p className="rounded-lg bg-slate-100 px-2 text-slate-700"> | ||
| {convertFloatToNDecimal(questionSummary.ctr.percentage, 1)}% | ||
| {convertFloatToNDecimal(questionSummary.ctr.percentage, 2)}% |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider using number formatting utilities for better readability.
For better readability of percentages, especially in different locales, consider using Intl.NumberFormat.
- {convertFloatToNDecimal(questionSummary.ctr.percentage, 2)}%
+ {new Intl.NumberFormat(undefined, {
+ minimumFractionDigits: 2,
+ maximumFractionDigits: 2,
+ style: 'percent',
+ }).format(questionSummary.ctr.percentage / 100)}📝 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.
| {convertFloatToNDecimal(questionSummary.ctr.percentage, 2)}% | |
| {new Intl.NumberFormat(undefined, { | |
| minimumFractionDigits: 2, | |
| maximumFractionDigits: 2, | |
| style: 'percent', | |
| }).format(questionSummary.ctr.percentage / 100)} |
| <div> | ||
| <p className="rounded-lg bg-slate-100 px-2 text-slate-700"> | ||
| {convertFloatToNDecimal(questionSummary[group]?.percentage, 1)}% | ||
| {convertFloatToNDecimal(questionSummary[group]?.percentage, 2)}% |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider UX implications of showing 2 decimal places.
While more precise, showing 2 decimal places in percentages might not always improve readability. Consider:
- Using 1 decimal place for larger numbers (>10%)
- 2 decimal places only for small percentages (<10%)
Here's a more nuanced approach:
- {convertFloatToNDecimal(questionSummary[group]?.percentage, 2)}%
+ {questionSummary[group]?.percentage >= 10
+ ? convertFloatToNDecimal(questionSummary[group]?.percentage, 1)
+ : convertFloatToNDecimal(questionSummary[group]?.percentage, 2)}%📝 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.
| {convertFloatToNDecimal(questionSummary[group]?.percentage, 2)}% | |
| {questionSummary[group]?.percentage >= 10 | |
| ? convertFloatToNDecimal(questionSummary[group]?.percentage, 1) | |
| : convertFloatToNDecimal(questionSummary[group]?.percentage, 2)}% |
gupta-piyush19
deleted the
fix/survey-summary-multi-choice-calculation
branch
2 participants
What does this PR do?
fixes inconsistencies in survey summary
Fixes # (issue)
How should this be tested?
Checklist
Required
pnpm buildconsole.logsgit pull origin mainAppreciated
Summary by CodeRabbit
Release Notes
New Features
Improvements
Bug Fixes