feat: adds contact info question type#3176
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎ 2 Skipped Deployments
|
WalkthroughThe pull request introduces updates to TypeScript declaration files for improved documentation clarity and adds components for managing address and contact information questions. The Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (2)
🔇 Additional comments (1)
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: 19
🧹 Outside diff range and nitpick comments (20)
packages/ui/ArrayResponse/index.tsx (1)
Line range hint
5-20: Consider handling empty arrays or all-empty-string arraysThe current implementation doesn't handle the case where the
valuearray is empty or contains only empty strings. In such cases, the component will render nothing, which might not be the desired behavior.Consider adding a check for empty arrays or all-empty-string arrays and render a placeholder or default message in such cases. Here's a suggested implementation:
export const ArrayResponse = ({ value }: ArrayResponseProps) => { + const nonEmptyValues = value.filter(item => item.trim() !== ''); + + if (nonEmptyValues.length === 0) { + return <div className="my-1 font-normal text-slate-500">No data available</div>; + } + return ( <div className="my-1 font-normal text-slate-700" dir="auto"> - {value.map( + {nonEmptyValues.map( (item, index) => - item && ( - <div key={index}> - {item} - <br /> - </div> - ) + <div key={index}> + {item} + <br /> + </div> )} </div> ); };This change filters out empty strings, handles the case of an empty array or all-empty-string array, and simplifies the rendering logic.
packages/surveys/src/components/general/Input.tsx (1)
8-18: LGTM: Component implementation is solid. Consider adding JSDoc comments.The
Inputcomponent is well-implemented:
- It correctly uses the
classNameprop and spreads other props.- The
cnutility is used effectively for class name management.- Styles cover various states including focus, disabled, and dark mode.
Consider adding JSDoc comments to describe the component's purpose and props. This would improve the component's documentation and make it easier for other developers to use. For example:
/** * A reusable Input component that extends the standard HTML input element. * * @param {InputProps} props - The props for the Input component. * @param {string} [props.className] - Additional CSS classes to apply to the input. * @returns {JSX.Element} The rendered Input component. */ export const Input = ({ className, ...props }: InputProps) => { // ... (rest of the implementation) };apps/web/app/(app)/environments/[environmentId]/surveys/[surveyId]/(analysis)/summary/components/ContactInfoSummary.tsx (1)
39-70: Enhance accessibility with ARIA labels.While the component structure is good, adding ARIA labels to key elements would improve accessibility for screen readers.
Consider adding ARIA labels to the grid and its rows. For example:
<div className="grid h-10 grid-cols-4 items-center border-y border-slate-200 bg-slate-100 text-sm font-bold text-slate-600" role="grid" aria-label="Contact Information Responses"> {/* ... grid headers ... */} </div> {questionSummary.samples.map((response, index) => ( <div key={response.id} className="grid grid-cols-4 items-center border-b border-slate-100 py-2 text-sm text-slate-800 last:border-transparent md:text-base" role="row" aria-rowindex={index + 2} // +2 because the header row is index 1 > {/* ... row content ... */} </div> ))}This will provide more context to screen reader users navigating the response list.
apps/web/app/(app)/environments/[environmentId]/surveys/[surveyId]/(analysis)/responses/components/ResponseDataView.tsx (1)
33-41: Rename variable and consider using Object.fromEntries for clarityThe function looks good overall, but there's a minor issue:
- The variable name
addressKeysis misleading as it contains contact info fields, not address fields.Also, consider using
Object.fromEntriesfor a more declarative approach:Here's a suggested improvement:
const formatContactInfoData = (responseValue: TResponseDataValue): Record<string, string> => { const contactInfoKeys = ["firstName", "lastName", "email", "phone", "company"]; return Array.isArray(responseValue) ? Object.fromEntries(contactInfoKeys.map((key, index) => [key, responseValue[index] || ''])) : {}; };This change improves readability and maintains the same functionality.
packages/surveys/src/components/questions/AddressQuestion.tsx (4)
50-52: LGTM: Improved state management with useMemo.The use of
useMemoto create asafeValuearray is a good optimization. It ensures that the array is only recreated when thevalueprop changes, potentially reducing unnecessary re-renders.Consider adding a type annotation to
safeValuefor improved type safety:const safeValue = useMemo<string[]>(() => { return Array.isArray(value) ? value : ["", "", "", "", "", ""]; }, [value]);
50-81: LGTM: Improved field configuration with centralized array.The new
fieldsarray is a great improvement in code organization and maintainability. It centralizes the configuration of input fields, making it easier to manage and extend the form in the future.Consider defining a type for the field objects to improve type safety:
type Field = { id: string; placeholder: string; show?: boolean; required?: boolean; }; const fields: Field[] = [ // ... existing field definitions ];This will help catch potential errors if the structure of the field objects changes in the future.
83-92: LGTM: Consolidated input handling with room for optimization.The new
handleChangefunction effectively consolidates input handling into a single, reusable function. It correctly updates the state while preserving values for other fields.Consider optimizing the performance by moving the
findIndexoperation outside themapfunction:const handleChange = (fieldId: string, fieldValue: string) => { const fieldIndex = fields.findIndex((f) => f.id === fieldId); const newValue = fields.map((field, index) => { if (index === fieldIndex) { return fieldValue; } const existingValue = safeValue?.[index] || ""; return field.show ? existingValue : ""; }); onChange({ [question.id]: newValue }); };This avoids running
findIndexfor each iteration of themapfunction, potentially improving performance for forms with many fields.
122-152: LGTM: Improved input rendering with dynamic requirements.The updates to the input field rendering are well-implemented:
- Using the
Inputcomponent improves consistency and reusability.- The
isFieldRequiredfunction provides clear logic for determining field requirements.- The rendering is now more dynamic, based on the
showproperty of each field.Consider adding an
aria-requiredattribute to improve accessibility:<Input // ... other props aria-required={isFieldRequired()} />This will ensure screen readers correctly announce whether a field is required or not.
packages/ui/SingleResponseCard/components/RenderResponse.tsx (2)
113-115: Approved: Address case now uses ArrayResponseThe change to use
ArrayResponsefor address type questions is consistent with the import changes and suggests a more generic handling of address data. This is a good improvement in terms of code reusability.For consistency with other cases in the switch statement, consider adding a break statement after the return:
case TSurveyQuestionTypeEnum.Address: if (Array.isArray(responseData)) { return <ArrayResponse value={responseData} />; } + break;
116-118: Approved: New ContactInfo case addedThe addition of the
ContactInfocase is consistent with the PR objective of adding a new contact info question type. The implementation usingArrayResponseis appropriate and consistent with theAddresscase.Consider the following improvements for consistency and robustness:
- Add a break statement after the return for consistency with other cases:
case TSurveyQuestionTypeEnum.ContactInfo: if (Array.isArray(responseData)) { return <ArrayResponse value={responseData} />; } + break;
- Add a fallback for non-array data to handle potential edge cases:
case TSurveyQuestionTypeEnum.ContactInfo: if (Array.isArray(responseData)) { return <ArrayResponse value={responseData} />; } + return <p className="ph-no-capture my-1 font-normal text-slate-700">{String(responseData)}</p>; break;packages/lib/utils/questions.tsx (1)
232-245: LGTM: New ContactInfo question type added with suggestions.The new ContactInfo question type is well-structured and follows the existing pattern. However, consider the following suggestions:
- Making all fields required by default might be too restrictive. Consider allowing some fields (e.g., phone, company) to be optional by default.
- Add input validation for email and phone fields to ensure data integrity.
- Consider adding a comment explaining the purpose and usage of this new question type.
Here's a suggested modification:
{ id: QuestionId.ContactInfo, label: "Contact Info", description: "Allow respondents to provide their contact info", icon: ContactIcon, preset: { headline: { default: "Contact Info" }, firstName: { show: true, required: true }, lastName: { show: true, required: true }, email: { show: true, required: true, validation: "email" }, phone: { show: true, required: false, validation: "phone" }, company: { show: true, required: false }, } as Partial<TSurveyContactInfoQuestion>, }This suggestion makes phone and company optional by default and adds placeholders for email and phone validation.
packages/surveys/src/components/general/QuestionConditional.tsx (1)
301-314: LGTM: New conditional rendering case for ContactInfoQuestion.The new conditional rendering case for
ContactInfoQuestionis correctly implemented and follows the existing pattern. All necessary props are passed to the component.For consistency with most other question types, consider adding the
autoFocusEnabledprop:<ContactInfoQuestion question={question} value={Array.isArray(value) ? value : undefined} onChange={onChange} onSubmit={onSubmit} onBack={onBack} isFirstQuestion={isFirstQuestion} isLastQuestion={isLastQuestion} languageCode={languageCode} ttc={ttc} setTtc={setTtc} currentQuestionId={currentQuestionId} + autoFocusEnabled={autoFocusEnabled} />apps/web/app/(app)/environments/[environmentId]/surveys/[surveyId]/(analysis)/responses/components/ResponseTableColumns.tsx (2)
38-53: LGTM with a minor suggestion for the default case.The
getContactInfoFieldLabelfunction is well-implemented and follows the existing pattern in the codebase. It covers all the necessary fields for contact information.Consider handling the default case to return a fallback value or throw an error for unexpected fields:
const getContactInfoFieldLabel = (field: string) => { switch (field) { case "firstName": return "First Name"; case "lastName": return "Last Name"; case "email": return "Email"; case "phone": return "Phone"; case "company": return "Company"; default: - break; + return field; // or throw new Error(`Unexpected field: ${field}`); } };
108-130: LGTM with a suggestion to address the static analysis hint.The implementation for the "contactInfo" case in the
getQuestionColumnsDatafunction is well-structured and consistent with the existing patterns in the codebase. It correctly handles the rendering of contact information fields and their values.To address the static analysis hint and improve code safety, consider wrapping the
contactInfoFieldsdeclaration in a block:case "contactInfo": { + const contactInfoFields = ["firstName", "lastName", "email", "phone", "company"]; return contactInfoFields.map((contactInfoField) => { // ... rest of the implementation }); }This change prevents other switch clauses from erroneously accessing the
contactInfoFieldsdeclaration.🧰 Tools
Biome
[error] 109-109: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.The declaration is defined in this switch clause:
Unsafe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
apps/web/app/(app)/environments/[environmentId]/surveys/[surveyId]/(analysis)/summary/components/SummaryList.tsx (1)
282-292: LGTM: Conditional block for ContactInfoSummaryThe new conditional block for rendering the
ContactInfoSummarycomponent is correctly implemented and follows the existing pattern in the file. This addition successfully integrates the new contact info question type into the summary list.For consistency with other question type checks, consider using the full enum value:
-if (questionSummary.type === TSurveyQuestionTypeEnum.ContactInfo) { +if (questionSummary.type === TSurveyQuestionTypeEnum.ContactInfo) {This change isn't strictly necessary but would align with the style used for other question types in this file.
packages/email/components/survey/preview-email-template.tsx (1)
464-482: Implementation looks good, with room for enhancements.The new
ContactInfocase is well-structured and consistent with other question types. Good job on maintaining code consistency and using localization for the headline and subheader.Consider the following enhancements to make the component more robust and user-friendly:
- Make input fields customizable based on the question configuration. This allows survey creators to choose which fields to include.
- Add visual indicators for required fields, if applicable.
- Implement specific styling or input types for email and phone fields to improve user experience.
Here's a sample implementation of these suggestions:
case TSurveyQuestionTypeEnum.ContactInfo: return ( <EmailTemplateWrapper styling={styling} surveyUrl={url}> <Text className="text-question-color m-0 mr-8 block p-0 text-base font-semibold leading-6"> {getLocalizedValue(firstQuestion.headline, defaultLanguageCode)} </Text> <Text className="text-question-color m-0 block p-0 text-sm font-normal leading-6"> {getLocalizedValue(firstQuestion.subheader, defaultLanguageCode)} </Text> - {["First Name", "Last Name", "Email", "Phone", "Company"].map((label) => ( + {firstQuestion.fields.map((field) => ( <Section - className="border-input-border-color bg-input-color rounded-custom mt-4 block h-10 w-full border border-solid py-2 pl-2 text-slate-400" - key={label}> - {label} + className={`border-input-border-color bg-input-color rounded-custom mt-4 block h-10 w-full border border-solid py-2 pl-2 text-slate-400 ${ + field.required ? 'border-red-500' : '' + }`} + key={field.id}> + {field.label} + {field.required && <span className="text-red-500">*</span>} + {field.type === 'email' && <span className="text-xs"> ([email protected])</span>} + {field.type === 'phone' && <span className="text-xs"> (e.g., +1 123-456-7890)</span>} </Section> ))} <EmailFooter /> </EmailTemplateWrapper> );This implementation assumes that the
firstQuestionobject has afieldsarray with each field's configuration. You'll need to update theTSurveyQuestionTypeEnum.ContactInfotype definition accordingly.packages/database/data-migrations/20240924123456_migrate_address_question/data-migration.ts (1)
10-10: Review the transaction timeout durationThe transaction timeout is set to 30 minutes. Ensure that this duration is appropriate for the data volume you expect to process. Long-running transactions may hold locks and impact database performance. Consider if a shorter timeout or processing in smaller batches would be more efficient.
packages/surveys/src/components/questions/ContactInfoQuestion.tsx (2)
121-131: Avoid defining functions inside loops for better performance.The
isFieldRequiredfunction is defined inside thefields.maploop, causing a new instance of the function to be created on each iteration. This can negatively impact performance. It's better to define the function outside the loop.Move
isFieldRequiredoutside the loop:- {fields.map((field, index) => { - const isFieldRequired = () => { + const isFieldRequired = (field: typeof fields[0]) => { if (field.required) { return true; } // additional logic... return false; }; + return fields.map((field, index) => { // use isFieldRequired(field) here
90-100: Use the correct event type inhandleSubmitfor better type safety.The
handleSubmitfunction currently types the event asEvent, which is generic. It's better to use the specificFormEventtype for form submissions.Update the event type:
- const handleSubmit = (e: Event) => { + const handleSubmit = (e: JSX.TargetedEvent<HTMLFormElement, Event>) => {packages/types/surveys/types.ts (1)
520-524: Consider renaming 'ZSurveyShowRequiredToggle' for clarityThe name
ZSurveyShowRequiredTogglemay not fully convey its purpose. Consider renaming it toZSurveyFieldDisplayOptionsorZSurveyFieldVisibilityto enhance readability and maintainability.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (21)
- apps/web/app/(app)/(survey-editor)/environments/[environmentId]/surveys/[surveyId]/edit/components/AddressQuestionForm.tsx (3 hunks)
- apps/web/app/(app)/(survey-editor)/environments/[environmentId]/surveys/[surveyId]/edit/components/ContactInfoQuestionForm.tsx (1 hunks)
- apps/web/app/(app)/(survey-editor)/environments/[environmentId]/surveys/[surveyId]/edit/components/LogicEditor.tsx (1 hunks)
- apps/web/app/(app)/(survey-editor)/environments/[environmentId]/surveys/[surveyId]/edit/components/QuestionCard.tsx (3 hunks)
- apps/web/app/(app)/environments/[environmentId]/surveys/[surveyId]/(analysis)/responses/components/ResponseDataView.tsx (2 hunks)
- apps/web/app/(app)/environments/[environmentId]/surveys/[surveyId]/(analysis)/responses/components/ResponseTableColumns.tsx (2 hunks)
- apps/web/app/(app)/environments/[environmentId]/surveys/[surveyId]/(analysis)/summary/components/AddressSummary.tsx (3 hunks)
- apps/web/app/(app)/environments/[environmentId]/surveys/[surveyId]/(analysis)/summary/components/ContactInfoSummary.tsx (1 hunks)
- apps/web/app/(app)/environments/[environmentId]/surveys/[surveyId]/(analysis)/summary/components/SummaryList.tsx (2 hunks)
- packages/database/data-migrations/20240924123456_migrate_address_question/data-migration.ts (1 hunks)
- packages/database/package.json (1 hunks)
- packages/email/components/survey/preview-email-template.tsx (1 hunks)
- packages/lib/response/utils.ts (2 hunks)
- packages/lib/utils/questions.tsx (3 hunks)
- packages/surveys/src/components/general/Input.tsx (1 hunks)
- packages/surveys/src/components/general/QuestionConditional.tsx (2 hunks)
- packages/surveys/src/components/questions/AddressQuestion.tsx (3 hunks)
- packages/surveys/src/components/questions/ContactInfoQuestion.tsx (1 hunks)
- packages/types/surveys/types.ts (8 hunks)
- packages/ui/ArrayResponse/index.tsx (1 hunks)
- packages/ui/SingleResponseCard/components/RenderResponse.tsx (2 hunks)
🧰 Additional context used
Biome
apps/web/app/(app)/(survey-editor)/environments/[environmentId]/surveys/[surveyId]/edit/components/AddressQuestionForm.tsx
[error] 155-155: Missing key property for this element in iterable.
The order of the items may change, and having a key can help React identify which item was moved.
Check the React documentation.(lint/correctness/useJsxKeyInIterable)
apps/web/app/(app)/(survey-editor)/environments/[environmentId]/surveys/[surveyId]/edit/components/ContactInfoQuestionForm.tsx
[error] 142-142: Missing key property for this element in iterable.
The order of the items may change, and having a key can help React identify which item was moved.
Check the React documentation.(lint/correctness/useJsxKeyInIterable)
apps/web/app/(app)/environments/[environmentId]/surveys/[surveyId]/(analysis)/responses/components/ResponseTableColumns.tsx
[error] 109-109: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.The declaration is defined in this switch clause:
Unsafe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
🔇 Additional comments not posted (41)
packages/ui/ArrayResponse/index.tsx (2)
1-3: LGTM: Interface renamed appropriatelyThe interface has been renamed from
AddressResponsePropstoArrayResponseProps, which aligns well with the more generic use case of handling an array of strings. Thevalueproperty of typestring[]is appropriate for this purpose.
Line range hint
5-20: LGTM: Component renamed and implemented correctlyThe component has been renamed from
AddressResponsetoArrayResponse, which aligns well with the more generic use case of handling an array of strings. The implementation correctly renders each non-empty item in thevaluearray, and the use ofdir="auto"is good for supporting right-to-left languages.packages/surveys/src/components/general/Input.tsx (3)
1-2: LGTM: Imports are appropriate and necessary.The imports are well-chosen for the component's needs:
cnis likely a utility for managing class names.HTMLAttributesfrom Preact is used for type definitions.
4-6: LGTM: Interface definition is well-structured.The
InputPropsinterface is correctly defined:
- It extends
HTMLAttributes<HTMLInputElement>, allowing all standard input attributes.- The optional
classNameprop enables flexible styling.- Exporting the interface allows for reuse in other components if needed.
1-18: Overall assessment: Well-implemented, reusable Input component.This new
Inputcomponent is a solid addition to the codebase:
- It's flexible and reusable, which aligns well with the PR's objective of adding new question types.
- The use of TypeScript for type safety and the
cnutility for class management are good practices.- The component's design allows for easy customization and extension, making it suitable for various use cases, including potentially the new contact info question type.
The only suggestion for improvement is to add JSDoc comments for better documentation.
apps/web/app/(app)/environments/[environmentId]/surveys/[surveyId]/(analysis)/summary/components/AddressSummary.tsx (3)
6-6: LGTM: Import statement for ArrayResponse added.The addition of the
ArrayResponseimport aligns with the changes described in the AI summary. This change supports the refactoring fromAddressResponseto a more genericArrayResponsecomponent.
63-65: Verify ArrayResponse compatibility with address data.The change from
AddressResponsetoArrayResponselooks good and aligns with the refactoring mentioned in the AI summary. This modification suggests a more flexible approach for rendering different types of responses.To ensure the
ArrayResponsecomponent can properly handle and display address data, please verify its implementation:
Line range hint
1-78: Clarify relationship to new "Contact Info" question type.The changes in this file improve the rendering of array-type responses, which could potentially include contact information. However, it's not immediately clear how these modifications directly relate to the new "Contact Info" question type mentioned in the PR objectives.
Could you please clarify:
- How does the
AddressSummarycomponent relate to the new "Contact Info" question type?- Are there other files in this PR that more directly implement the new question type?
To help verify the implementation of the new question type, you could run the following script:
apps/web/app/(app)/environments/[environmentId]/surveys/[surveyId]/(analysis)/summary/components/ContactInfoSummary.tsx (3)
1-8: LGTM: Imports are well-organized and relevant.The imports are logically structured and include all necessary dependencies for the component's functionality.
10-15: LGTM: Well-defined props interface.The
ContactInfoSummaryPropsinterface is clearly defined with appropriate types, ensuring type safety for the component's props.
17-77: LGTM: Well-structured and functional component.The
ContactInfoSummarycomponent is well-implemented, with clear structure, appropriate use of sub-components, and good handling of different response types.apps/web/app/(app)/environments/[environmentId]/surveys/[surveyId]/(analysis)/responses/components/ResponseDataView.tsx (2)
57-59: Approve changes and suggest minor improvement for consistencyThe addition of the "contactInfo" case is correct and consistent with the existing code structure. Good job!
For improved consistency with other cases, consider destructuring the result:
case "contactInfo": responseData = { ...responseData, ...formatContactInfoData(responseValue) }; break;This change aligns the "contactInfo" case more closely with the "address" case, enhancing code readability and maintainability.
Line range hint
33-59: Summary: Good implementation, ensure thorough testingThe changes to support the new "contactInfo" question type are well-implemented and consistent with the existing code structure. Good job on maintaining code quality and following established patterns!
To ensure the robustness of these changes:
- Verify that all parts of the application that interact with survey responses handle the new "contactInfo" type correctly.
- Add unit tests for the
formatContactInfoDatafunction and update existing tests forextractResponseData.- Perform integration tests to ensure the new question type works correctly in the survey creation and response viewing flows.
Run the following script to check for any other files that might need updates:
This will help identify any other areas of the codebase that might need attention due to the new question type.
✅ Verification successful
It appears that the
rgcommand does not recognize thetypescriptandtsxfile types in the current environment. To address this, we'll adjust the search commands to target the appropriate file extensions directly.Please run the updated script to gather the necessary information for verification.
The implementation of the new "contactInfo" question type is correctly isolated within
ResponseDataView.tsx, and no additional files require changes. The code integrates seamlessly with the existing codebase. Good job on ensuring that the addition is both effective and contained!🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Find files that might need updates for the new contactInfo type # Test: Search for files mentioning question types echo "Files mentioning question types:" rg --type typescript --type tsx "case.*:.*break" -g '!**/ResponseDataView.tsx' # Test: Search for files related to survey responses echo "\nFiles related to survey responses:" rg --type typescript --type tsx "TResponse|TSurvey" -g '!**/ResponseDataView.tsx'Length of output: 392
Script:
#!/bin/bash # Description: Find files that might need updates for the new contactInfo type # Search for TypeScript and TSX files mentioning question types echo "Files mentioning question types:" rg "case.*:.*break" -g "*.ts" -g "*.tsx" -g '!**/ResponseDataView.tsx' # Search for TypeScript and TSX files related to survey responses echo "\nFiles related to survey responses:" rg "TResponse|TSurvey" -g "*.ts" -g "*.tsx" -g '!**/ResponseDataView.tsx'Length of output: 217689
packages/database/package.json (1)
51-52: LGTM! Consider including in version-specific migration.The new data migration script for the address question has been added correctly, following the established pattern for data migration scripts in this package.
Consider including this new migration script in an existing version-specific migration command (e.g.,
data-migration:v2.5) or creating a new one (e.g.,data-migration:v2.6) to ensure it's run as part of a complete version update. This would help maintain consistency in how migrations are applied across different versions of the application.Example:
"data-migration:v2.5": "pnpm data-migration:remove-dismissed-value-inconsistency", + "data-migration:v2.6": "pnpm data-migration:add-display-id-to-response && pnpm data-migration:address-question"To ensure the migration script is properly placed and accessible, let's verify its existence:
packages/surveys/src/components/questions/AddressQuestion.tsx (4)
4-4: LGTM: Import changes enhance modularity and performance.The addition of the
Inputcomponent import and theuseMemohook are positive changes:
- Using a reusable
Inputcomponent improves code modularity and consistency.- The
useMemohook can help optimize performance by memoizing computed values.These changes align well with React best practices.
Also applies to: 9-9
Line range hint
15-26: Verify impact of removed autoFocus props on accessibility.The
autoFocusandautoFocusEnabledprops have been removed from theAddressQuestionPropsinterface. While this simplifies the component's API, it's important to ensure that this change doesn't negatively impact the form's accessibility, particularly for keyboard users.Please confirm that:
- The form can still be navigated effectively using a keyboard.
- The first input field receives focus when the form is rendered, if appropriate.
Consider running an accessibility audit to ensure these changes don't introduce any issues.
94-104: LGTM: Improved submission handling with empty check.The updates to the
handleSubmitfunction are well-implemented:
- The check for all empty strings ensures consistent data submission.
- The time-to-completion (ttc) metric is correctly updated.
- The function handles both empty and non-empty submissions appropriately.
These changes improve the overall robustness of the form submission process.
Line range hint
1-173: Overall assessment: Well-implemented changes with minor suggestions for improvement.This PR introduces several positive changes to the
AddressQuestioncomponent:
- Improved modularity with the use of the
Inputcomponent.- Better state management using
useMemo.- Centralized field configuration with the
fieldsarray.- Consolidated input handling with the
handleChangefunction.- More robust form submission logic.
The changes enhance the component's maintainability, performance, and flexibility. The suggestions provided in the review comments are minor and aimed at further improving type safety, performance, and accessibility.
Before merging, please:
- Address the suggestions in the review comments, particularly those related to accessibility.
- Ensure that removing the
autoFocusprops doesn't negatively impact form usability.- Consider running an accessibility audit to catch any potential issues.
Great work on improving this component!
packages/ui/SingleResponseCard/components/RenderResponse.tsx (2)
Line range hint
1-180: Summary: ContactInfo question type successfully implementedThe changes in this file successfully implement the new ContactInfo question type and refactor the Address case to use a more generic
ArrayResponsecomponent. This approach improves code reusability and consistency in handling structured data for survey responses.Key points:
- The
ArrayResponseimport and usage are appropriate for both Address and ContactInfo cases.- The implementation is consistent with the PR objectives.
- Minor improvements for consistency (adding break statements) and robustness (handling non-array data) have been suggested.
Overall, the changes are well-integrated into the existing code structure and represent a positive improvement in the survey response rendering logic.
16-16: Verify removal of AddressResponse importThe addition of
ArrayResponseimport is appropriate for the changes made in the component. However, please ensure that theAddressResponseimport has been removed if it's no longer used.To confirm the removal of the
AddressResponseimport, run the following script:✅ Verification successful
AddressResponse import successfully removed
The
ArrayResponseimport has been added correctly, and theAddressResponseimport has been removed as intended.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for the presence of AddressResponse import # Test: Search for AddressResponse import rg --type typescript 'import.*AddressResponse' packages/ui/SingleResponseCard/components/RenderResponse.tsx # If the above command returns no results, it confirms that the AddressResponse import has been removed.Length of output: 144
Script:
#!/bin/bash # Description: Check for the presence of AddressResponse import without specifying file type # Test: Search for AddressResponse import in the specific file rg 'import.*AddressResponse' packages/ui/SingleResponseCard/components/RenderResponse.tsx # If the above command returns no results, it confirms that the AddressResponse import has been removed.Length of output: 91
packages/lib/utils/questions.tsx (3)
6-6: LGTM: New imports for ContactInfo question type.The new imports for
ContactIconandTSurveyContactInfoQuestionare correctly added and align with the existing import structure.Also applies to: 25-25
224-229: Consider the impact of making all address fields required.The changes to the Address question type preset now make all fields (addressLine1, addressLine2, city, state, zip, country) required. This is a significant change that could impact existing surveys and user expectations.
- Was this change intentional?
- How will this affect existing surveys that use the Address question type?
- Is there a migration plan for existing data?
- Should we consider adding a comment explaining the rationale behind this change?
Please clarify the reasoning behind this change and consider its potential impacts.
Line range hint
1-245: Overall assessment: Good addition with some considerations.The changes to this file introduce a new ContactInfo question type and modify the existing Address question type. While these additions enhance the survey capabilities, there are a few points to consider:
- The Address question type now requires all fields, which might impact existing surveys. This needs clarification and possibly a migration plan.
- The new ContactInfo question type is well-structured but could benefit from more flexible field requirements and input validation.
- These changes might require updates to documentation and possibly the UI for creating/editing surveys.
Please address the comments in the individual review sections, particularly regarding the Address question type changes and the suggestions for the ContactInfo type.
To ensure these changes don't negatively impact the existing codebase, consider running the following checks:
These checks will help identify any potential conflicts or areas that might need additional updates due to these changes.
packages/surveys/src/components/general/QuestionConditional.tsx (2)
5-5: LGTM: New import statement for ContactInfoQuestion.The import statement for
ContactInfoQuestionis correctly added and follows the existing pattern for importing question components.
Line range hint
1-314: Summary: ContactInfoQuestion successfully integrated.The changes to add support for the new
ContactInfoQuestiontype are well-implemented and follow the existing patterns in the file. The new question type is correctly imported and integrated into the conditional rendering logic.To ensure the new question type is fully supported, please run the following verification script:
This script will help verify that the
ContactInfoQuestionis properly exported, the enum is updated, and there are no remaining TODO comments related to this new question type.✅ Verification successful
ContactInfoQuestion integration verified successfully.
The
ContactInfoQuestiontype is correctly exported, included inTSurveyQuestionTypeEnum, and no related TODO comments were found.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that ContactInfoQuestion is properly integrated and exported. # Test 1: Check if ContactInfoQuestion is exported from the questions directory grep -R "export.*ContactInfoQuestion" packages/surveys/src/components/questions/ # Test 2: Verify that TSurveyQuestionTypeEnum includes ContactInfo grep -R "ContactInfo" packages/surveys/src/types/ # Test 3: Check for any TODO comments related to ContactInfoQuestion grep -R "TODO.*ContactInfoQuestion" packages/surveys/src/Length of output: 357
Script:
#!/bin/bash # Description: Locate the definition of TSurveyQuestionTypeEnum and verify it includes ContactInfo. # Search for the definition of TSurveyQuestionTypeEnum rg "enum\s+TSurveyQuestionTypeEnum" packages/surveys/src/ # If found, check if ContactInfo is one of the enum members rg "enum\s+TSurveyQuestionTypeEnum" packages/surveys/src/ -A 10 | grep "ContactInfo"Length of output: 144
Script:
#!/bin/bash # Description: Comprehensive search for TSurveyQuestionTypeEnum and related definitions. # Search for any occurrence of TSurveyQuestionTypeEnum in the entire repository rg "TSurveyQuestionTypeEnum" . # Search for enums that include ContactInfo as a member rg "enum\s+\w+" . | grep -A 10 "ContactInfo" # Search for type aliases or interfaces that might define question types rg "type\s+\w+|interface\s+\w+" . | grep -E "ContactInfo|QuestionType"Length of output: 54311
apps/web/app/(app)/environments/[environmentId]/surveys/[surveyId]/(analysis)/responses/components/ResponseTableColumns.tsx (1)
Line range hint
1-359: Summary: New "Contact Info" question type successfully implementedThe changes in this PR effectively introduce the new "Contact Info" question type to the Formbricks survey platform, addressing the issue from the internal tracker (issue number 168). The implementation is consistent with existing patterns in the codebase and maintains good code quality.
Key points:
- A new
getContactInfoFieldLabelfunction has been added to handle contact info fields.- The
getQuestionColumnsDatafunction has been updated to support the new "contactInfo" case.- The changes integrate well with the existing code structure and follow established patterns.
The new feature can be tested by creating a new survey or editing an existing one, where the Contact Info question type should now be visible among the available questions.
To ensure the new question type is properly integrated, please run the following verification script:
This script will help verify that the new question type is properly integrated into the codebase and that there are no remaining TODO items related to this feature.
✅ Verification successful
[/run_scripts]
Contact Info Integration Verified Successfully
All verification tests confirm that the new "Contact Info" question type is properly integrated into the codebase. The
QUESTIONS_ICON_MAPincludes the necessary entry, and there are no outstanding TODO comments related to this feature.
- The "contactInfo" type is correctly implemented across relevant components.
- The Contact Info icon is defined and utilized as expected.
- No pending tasks or comments remain for this feature.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the integration of the new "Contact Info" question type # Test 1: Check if the new question type is imported and used in the survey creation/editing components echo "Checking for Contact Info question type integration:" rg --type typescript 'contactInfo' # Test 2: Verify that the Contact Info icon is properly defined echo "\nChecking for Contact Info icon definition:" rg --type typescript 'QUESTIONS_ICON_MAP.*contactInfo' # Test 3: Look for any TODO comments related to the new feature echo "\nChecking for any remaining TODO comments:" rg --type typescript 'TODO.*contact.*info'Length of output: 552
Script:
#!/bin/bash # Description: Verify the integration of the new "Contact Info" question type # Test 1: Check if the new question type is imported and used in the survey creation/editing components echo "Checking for Contact Info question type integration:" rg 'contactInfo' --glob '*.ts' --glob '*.tsx' # Test 2: Verify that the Contact Info icon is properly defined echo "\nChecking for Contact Info icon definition:" rg 'QUESTIONS_ICON_MAP.*contactInfo' --glob '*.ts' --glob '*.tsx' # Test 3: Look for any TODO comments related to the new feature echo "\nChecking for any remaining TODO comments:" rg 'TODO.*contact.*info' --glob '*.ts' --glob '*.tsx'Length of output: 2465
🧰 Tools
Biome
[error] 109-109: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.The declaration is defined in this switch clause:
Unsafe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
apps/web/app/(app)/environments/[environmentId]/surveys/[surveyId]/(analysis)/summary/components/SummaryList.tsx (2)
11-11: LGTM: Import statement for ContactInfoSummaryThe import statement for the new
ContactInfoSummarycomponent is correctly added and follows the existing import pattern in the file. This is in line with the PR objective of adding a new question type for contact information.
Line range hint
1-292: Summary: Successfully integrated ContactInfo question typeThe changes in this file effectively add support for the new ContactInfo question type in the SummaryList component. The implementation follows existing patterns, maintaining consistency with other question types. These changes align well with the PR objectives and should seamlessly integrate the new functionality without disrupting existing features.
A few points to note:
- The import statement is correctly added.
- The new conditional block for rendering ContactInfoSummary is properly implemented.
- No existing code was modified, minimizing the risk of unintended side effects.
Overall, these changes appear to be well-implemented and ready for integration.
apps/web/app/(app)/(survey-editor)/environments/[environmentId]/surveys/[surveyId]/edit/components/LogicEditor.tsx (2)
Line range hint
1-559: Verify handling of the new contactInfo condition typeWhile the addition of the
contactInfocondition should automatically be included in the logic editor UI, please verify the following:
- Ensure that the
LogicEditorcomponent correctly handles the newcontactInfocondition type.- Check if the
getLogicDisplayValueandgetOptionPreviewfunctions need any updates to support thecontactInfotype.- Verify that the logic for rendering and updating conditions in the component works correctly for the new type.
To assist with this verification, you can run the following script:
#!/bin/bash # Description: Verify handling of contactInfo condition type in LogicEditor component # Check for any special handling of contactInfo in the component echo "Checking for special handling of contactInfo:" grep -n -E "contactInfo|question.type" apps/web/app/\(app\)/\(survey-editor\)/environments/\[environmentId\]/surveys/\[surveyId\]/edit/components/LogicEditor.tsx # Check if getLogicDisplayValue and getOptionPreview functions need updates echo "Checking getLogicDisplayValue and getOptionPreview functions:" sed -n '/getLogicDisplayValue/,/^ }/p' apps/web/app/\(app\)/\(survey-editor\)/environments/\[environmentId\]/surveys/\[surveyId\]/edit/components/LogicEditor.tsx echo "---" sed -n '/getOptionPreview/,/^ }/p' apps/web/app/\(app\)/\(survey-editor\)/environments/\[environmentId\]/surveys/\[surveyId\]/edit/components/LogicEditor.tsx # Check for any TODO comments related to handling different question types echo "Checking for TODOs related to question type handling:" grep -n -E "TODO.*question.type" apps/web/app/\(app\)/\(survey-editor\)/environments/\[environmentId\]/surveys/\[surveyId\]/edit/components/LogicEditor.tsxThis script will help you identify any areas that might need attention to fully support the new
contactInfocondition type in theLogicEditorcomponent.
84-84: LGTM: New condition type added correctly.The addition of the
contactInfocondition with "submitted" and "skipped" values is consistent with the existing structure and aligns with the PR objective of adding a contact info question type.To ensure this change is sufficient for the new contact info question type, please run the following verification:
This script will help verify if any additional changes are needed in this file or other parts of the application to fully support the new contact info question type.
✅ Verification successful
Verified:
contactInfocondition added correctly.The
contactInfocondition with values"submitted"and"skipped"has been successfully integrated into theLogicEditor.tsxcomponent. No additional TODOs or FIXMEs related tocontactInfowere found, and the change aligns with the existing structure and functionality.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify if additional changes are needed for the new contact info question type # Check if the contactInfo type is used in the component echo "Checking usage of contactInfo in LogicEditor.tsx:" grep -n "contactInfo" apps/web/app/\(app\)/\(survey-editor\)/environments/\[environmentId\]/surveys/\[surveyId\]/edit/components/LogicEditor.tsx # Check if there are any TODOs or FIXMEs related to contactInfo echo "Checking for TODOs or FIXMEs related to contactInfo:" grep -n -E "TODO|FIXME" apps/web/app/\(app\)/\(survey-editor\)/environments/\[environmentId\]/surveys/\[surveyId\]/edit/components/LogicEditor.tsx | grep -i "contactInfo" # Check for any other files that might need updates for the new question type echo "Checking for other files that might need updates:" grep -r "questionType" apps/web --include="*.ts" --include="*.tsx" | grep -v "LogicEditor.tsx"Length of output: 11178
packages/lib/response/utils.ts (2)
1307-1331: Verify the integration of ContactInfo with the rest of the codebase.The new ContactInfo case is well-integrated into the getQuestionWiseSummary function. To ensure full compatibility:
- Confirm that TSurveyQuestionTypeEnum has been updated to include ContactInfo.
- Check if any code relying on exhaustive switches over TSurveyQuestionTypeEnum needs updating.
- Verify that the use of VALUES_LIMIT is consistent with other question types.
Run the following script to help with verification:
#!/bin/bash # Description: Verify the integration of ContactInfo in the codebase # Check if TSurveyQuestionTypeEnum includes ContactInfo rg --type typescript "TSurveyQuestionTypeEnum.*ContactInfo" # Look for switch statements that might need updating rg --type typescript "switch.*TSurveyQuestionTypeEnum" # Verify consistent use of VALUES_LIMIT rg --type typescript "VALUES_LIMIT.*ContactInfo|ContactInfo.*VALUES_LIMIT"
1307-1331: Implementation for ContactInfo question type looks good, but needs verification.The new case for handling the ContactInfo question type is implemented consistently with other similar question types. However, please verify the following:
- Ensure that the
TSurveyQuestionSummaryContactInfotype is properly defined and imported.- Confirm that the ContactInfo data structure is indeed expected to be an array.
- Verify if any special processing of the ContactInfo data is required, or if the current implementation sufficiently meets the requirements.
To help with verification, you can run the following script:
✅ Verification successful
To proceed with the verification, please run the following updated scripts:
Verified: ContactInfo Type Implementation
The
TSurveyQuestionSummaryContactInfotype is properly defined inpackages/types/surveys/types.ts, and no special processing ofContactInfodata was detected elsewhere in the codebase.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the TSurveyQuestionSummaryContactInfo type and its usage # Check if the type is defined rg --type typescript "type TSurveyQuestionSummaryContactInfo" # Check for any special processing of ContactInfo data in other parts of the codebase rg --type typescript "ContactInfo.*process|process.*ContactInfo"Length of output: 300
Script:
#!/bin/bash # Description: Verify the TSurveyQuestionSummaryContactInfo type and its usage # Check if the type is defined rg --type ts "type TSurveyQuestionSummaryContactInfo" # Check for any special processing of ContactInfo data in other parts of the codebase rg --type ts "ContactInfo.*process|process.*ContactInfo"Length of output: 245
packages/database/data-migrations/20240924123456_migrate_address_question/data-migration.ts (1)
44-51: Ensure type safety when casting question to extended typeUsing a type assertion with
asmay bypass TypeScript's type checking and could lead to runtime errors if the expected properties are missing. Consider adding runtime checks or refining the types to ensure these properties exist before destructuring.packages/types/surveys/types.ts (8)
69-69: Enum updated with new question type 'ContactInfo'The
ContactInfovalue has been correctly added toTSurveyQuestionTypeEnum.
274-278: Added 'ZSurveyContactInfoLogic' for 'ContactInfo' question typeThe
ZSurveyContactInfoLogicextendsZSurveyLogicBaseappropriately, defining conditionssubmittedandskipped, consistent with other similar question types.
360-360: Included 'ZSurveyContactInfoLogic' in 'ZSurveyLogic' unionThe
ZSurveyContactInfoLogichas been correctly added to theZSurveyLogicunion type.
527-532: Updated 'ZSurveyAddressQuestion' to use 'ZSurveyShowRequiredToggle' for address fieldsThe address fields now use
ZSurveyShowRequiredToggleto specify visibility and requirement options, which enhances consistency and flexibility.
571-571: Included 'ZSurveyContactInfoQuestion' in 'ZSurveyQuestion' unionThe
ZSurveyContactInfoQuestionhas been correctly added to theZSurveyQuestionunion type.
595-595: Updated 'ZSurveyQuestionType' enum to include 'ContactInfo'The
ContactInfoquestion type has been appropriately added toZSurveyQuestionType.
1471-1471: Included 'ZSurveyQuestionSummaryContactInfo' in 'ZSurveyQuestionSummary' unionThe
ZSurveyQuestionSummaryContactInfohas been correctly added to the union typeZSurveyQuestionSummary.
536-545: Ensure proper validation for 'email' field in 'ZSurveyContactInfoQuestion'While the
ZSurveyShowRequiredTogglefor visibility and requirement, ensure that email values collected are properly validated as valid email addresses elsewhere in the codebase.Here's a script to check for email validation logic:
| <div className="max-h-[62vh] w-full overflow-y-auto"> | ||
| {questionSummary.samples.map((response) => { | ||
| return ( | ||
| <div | ||
| key={response.id} | ||
| className="grid grid-cols-4 items-center border-b border-slate-100 py-2 text-sm text-slate-800 last:border-transparent md:text-base"> | ||
| <div className="pl-4 md:pl-6"> | ||
| {response.person ? ( | ||
| <Link | ||
| className="ph-no-capture group flex items-center" | ||
| href={`/environments/${environmentId}/people/${response.person.id}`}> | ||
| <div className="hidden md:flex"> | ||
| <PersonAvatar personId={response.person.id} /> | ||
| </div> | ||
| <p className="ph-no-capture break-all text-slate-600 group-hover:underline md:ml-2"> | ||
| {getPersonIdentifier(response.person, response.personAttributes)} | ||
| </p> | ||
| </Link> | ||
| ) : ( | ||
| <div className="group flex items-center"> | ||
| <div className="hidden md:flex"> | ||
| <PersonAvatar personId="anonymous" /> | ||
| </div> | ||
| <p className="break-all text-slate-600 md:ml-2">Anonymous</p> | ||
| </div> | ||
| )} | ||
| </div> | ||
| <div className="ph-no-capture col-span-2 pl-6 font-semibold"> | ||
| <ArrayResponse value={response.value} /> | ||
| </div> | ||
|
|
||
| <div className="px-4 text-slate-500 md:px-6"> | ||
| {timeSince(new Date(response.updatedAt).toISOString())} | ||
| </div> | ||
| </div> | ||
| ); | ||
| })} |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider optimizing the response list rendering.
The current implementation might cause performance issues for large datasets. Consider implementing virtualization for the response list to improve performance with large numbers of responses.
You could use a library like react-window or react-virtualized to implement efficient rendering of large lists. Here's a basic example of how you might refactor this using react-window:
import { FixedSizeList as List } from 'react-window';
// ... (rest of the imports)
const Row = ({ index, style }) => {
const response = questionSummary.samples[index];
return (
<div style={style} className="grid grid-cols-4 items-center border-b border-slate-100 py-2 text-sm text-slate-800 last:border-transparent md:text-base">
{/* ... (rest of the row content) */}
</div>
);
};
export const ContactInfoSummary = ({ questionSummary, environmentId, survey, attributeClasses }: ContactInfoSummaryProps) => {
return (
<div className="rounded-xl border border-slate-200 bg-white shadow-sm">
{/* ... (QuestionSummaryHeader and grid header) */}
<div className="max-h-[62vh] w-full overflow-y-auto">
<List
height={600}
itemCount={questionSummary.samples.length}
itemSize={50}
width="100%"
>
{Row}
</List>
</div>
</div>
);
};This approach would significantly improve performance for large datasets by only rendering the visible items.
| {questionSummary.samples.map((response) => { | ||
| return ( | ||
| <div | ||
| key={response.id} | ||
| className="grid grid-cols-4 items-center border-b border-slate-100 py-2 text-sm text-slate-800 last:border-transparent md:text-base"> | ||
| <div className="pl-4 md:pl-6"> | ||
| {response.person ? ( | ||
| <Link | ||
| className="ph-no-capture group flex items-center" | ||
| href={`/environments/${environmentId}/people/${response.person.id}`}> | ||
| <div className="hidden md:flex"> | ||
| <PersonAvatar personId={response.person.id} /> | ||
| </div> | ||
| <p className="ph-no-capture break-all text-slate-600 group-hover:underline md:ml-2"> | ||
| {getPersonIdentifier(response.person, response.personAttributes)} | ||
| </p> | ||
| </Link> | ||
| ) : ( | ||
| <div className="group flex items-center"> | ||
| <div className="hidden md:flex"> | ||
| <PersonAvatar personId="anonymous" /> | ||
| </div> | ||
| <p className="break-all text-slate-600 md:ml-2">Anonymous</p> | ||
| </div> | ||
| )} | ||
| </div> | ||
| <div className="ph-no-capture col-span-2 pl-6 font-semibold"> | ||
| <ArrayResponse value={response.value} /> | ||
| </div> | ||
|
|
||
| <div className="px-4 text-slate-500 md:px-6"> | ||
| {timeSince(new Date(response.updatedAt).toISOString())} | ||
| </div> | ||
| </div> | ||
| ); | ||
| })} |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider extracting the response row into a separate component.
The response row rendering logic is quite complex. Extracting it into a separate component would improve readability and maintainability.
Here's how you could refactor this:
const ResponseRow = ({ response, environmentId }) => (
<div className="grid grid-cols-4 items-center border-b border-slate-100 py-2 text-sm text-slate-800 last:border-transparent md:text-base">
{/* ... (user column) */}
<div className="ph-no-capture col-span-2 pl-6 font-semibold">
<ArrayResponse value={response.value} />
</div>
<div className="px-4 text-slate-500 md:px-6">
{timeSince(new Date(response.updatedAt).toISOString())}
</div>
</div>
);
// In the main component:
{questionSummary.samples.map((response) => (
<ResponseRow key={response.id} response={response} environmentId={environmentId} />
))}This refactoring would make the main component more concise and easier to understand, while also making it easier to test and maintain the row rendering logic separately.
| async (transactionPrisma) => { | ||
| const surveysWithAddressQuestion = await transactionPrisma.survey.findMany({ | ||
| where: { | ||
| questions: { | ||
| array_contains: [{ type: "address" }], | ||
| }, | ||
| }, | ||
| }); | ||
|
|
||
| console.log(`Found ${surveysWithAddressQuestion.length.toString()} surveys with address questions`); | ||
|
|
||
| const updationPromises = surveysWithAddressQuestion.map((survey) => { | ||
| const updatedQuestions = survey.questions.map((question: TSurveyQuestion) => { | ||
| if (question.type === TSurveyQuestionTypeEnum.Address) { | ||
| // eslint-disable-next-line @typescript-eslint/no-unnecessary-condition -- addressLine1 is not defined for unmigrated surveys | ||
| if (question.addressLine1 !== undefined) { | ||
| return question; | ||
| } | ||
|
|
||
| const { | ||
| isAddressLine1Required, | ||
| isAddressLine2Required, | ||
| isCityRequired, | ||
| isStateRequired, | ||
| isZipRequired, | ||
| isCountryRequired, | ||
| ...rest | ||
| } = question as TSurveyAddressQuestion & { | ||
| isAddressLine1Required: boolean; | ||
| isAddressLine2Required: boolean; | ||
| isCityRequired: boolean; | ||
| isStateRequired: boolean; | ||
| isZipRequired: boolean; | ||
| isCountryRequired: boolean; | ||
| }; | ||
|
|
||
| return { | ||
| ...rest, | ||
| addressLine1: { show: true, required: isAddressLine1Required }, | ||
| addressLine2: { show: true, required: isAddressLine2Required }, | ||
| city: { show: true, required: isCityRequired }, | ||
| state: { show: true, required: isStateRequired }, | ||
| zip: { show: true, required: isZipRequired }, | ||
| country: { show: true, required: isCountryRequired }, | ||
| }; | ||
| } | ||
|
|
||
| return question; | ||
| }); | ||
|
|
||
| return transactionPrisma.survey.update({ | ||
| where: { | ||
| id: survey.id, | ||
| }, | ||
| data: { | ||
| questions: updatedQuestions, | ||
| }, | ||
| }); | ||
| }); | ||
|
|
||
| await Promise.all(updationPromises); | ||
|
|
||
| console.log("Data migration completed"); | ||
| }, | ||
| { | ||
| timeout: TRANSACTION_TIMEOUT, | ||
| } | ||
| ); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider processing surveys in batches to improve performance
Processing all surveys with address questions in a single transaction might lead to performance issues or transaction timeouts, especially if the number of surveys is large. Consider fetching and updating surveys in smaller batches to improve performance and reduce the risk of timeouts.
| if (question.addressLine1 !== undefined) { | ||
| return question; | ||
| } |
There was a problem hiding this comment.
Verify if checking addressLine1 is sufficient to determine migration status
Currently, the code checks if question.addressLine1 !== undefined to determine if a question has already been migrated. There might be cases where addressLine1 is defined but other new fields are not. It would be safer to check all the new address fields or set a migration flag to ensure complete migration.
| prisma.$disconnect().catch(handleDisconnectError); | ||
| }); |
There was a problem hiding this comment.
Await Prisma client disconnection to ensure proper cleanup
In the finally block, the call to prisma.$disconnect() is not awaited. This may result in the disconnection process not completing before the script exits. To ensure the client disconnects properly, consider awaiting this call.
Apply the following changes:
-.finally(() => {
- prisma.$disconnect().catch(handleDisconnectError);
+ .finally(async () => {
+ await prisma.$disconnect().catch(handleDisconnectError);
});Additionally, since await is used, the main function should be marked as async:
-function main(): void {
+async function main(): Promise<void> {Committable suggestion was skipped due to low confidence.
| useEffect(() => { | ||
| const allFieldsAreOptional = [ | ||
| question.firstName, | ||
| question.lastName, | ||
| question.email, | ||
| question.phone, | ||
| question.company, | ||
| ] | ||
| .filter((field) => field.show) | ||
| .every((field) => !field.required); | ||
|
|
||
| if (allFieldsAreOptional) { | ||
| updateQuestion(questionIdx, { required: false }); | ||
| } | ||
| // eslint-disable-next-line react-hooks/exhaustive-deps | ||
| }, [question.firstName, question.lastName, question.email, question.phone, question.company]); |
There was a problem hiding this comment.
Avoid disabling ESLint rules; ensure useEffect dependencies are complete
In the useEffect hook (lines 64-79), the react-hooks/exhaustive-deps ESLint rule is disabled. Disabling ESLint rules can hide potential issues. It's best to include all necessary dependencies in the dependency array to ensure the effect runs correctly.
Consider updating the dependency array to include all relevant dependencies:
- // eslint-disable-next-line react-hooks/exhaustive-deps
- }, [question.firstName, question.lastName, question.email, question.phone, question.company]);
+ }, [
+ question.firstName,
+ question.lastName,
+ question.email,
+ question.phone,
+ question.company,
+ questionIdx,
+ updateQuestion,
+ ]);📝 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.
| useEffect(() => { | |
| const allFieldsAreOptional = [ | |
| question.firstName, | |
| question.lastName, | |
| question.email, | |
| question.phone, | |
| question.company, | |
| ] | |
| .filter((field) => field.show) | |
| .every((field) => !field.required); | |
| if (allFieldsAreOptional) { | |
| updateQuestion(questionIdx, { required: false }); | |
| } | |
| // eslint-disable-next-line react-hooks/exhaustive-deps | |
| }, [question.firstName, question.lastName, question.email, question.phone, question.company]); | |
| useEffect(() => { | |
| const allFieldsAreOptional = [ | |
| question.firstName, | |
| question.lastName, | |
| question.email, | |
| question.phone, | |
| question.company, | |
| ] | |
| .filter((field) => field.show) | |
| .every((field) => !field.required); | |
| if (allFieldsAreOptional) { | |
| updateQuestion(questionIdx, { required: false }); | |
| } | |
| }, [ | |
| question.firstName, | |
| question.lastName, | |
| question.email, | |
| question.phone, | |
| question.company, | |
| questionIdx, | |
| updateQuestion, | |
| ]); |
| <tr className="text-slate-900"> | ||
| <td className="py-2 text-sm">{field.label}</td> | ||
| <td className="py-"> | ||
| <Switch | ||
| checked={field.show} | ||
| onCheckedChange={(show) => { | ||
| updateQuestion(questionIdx, { | ||
| [field.id]: { | ||
| show, | ||
| required: field.required, | ||
| }, | ||
| // when show changes, and the field is required, the question should be required | ||
| ...(show && field.required && { required: true }), | ||
| }); | ||
| }} | ||
| disabled={ | ||
| // if all the other fields are hidden, this should be disabled | ||
| fields | ||
| .filter((currentField) => currentField.id !== field.id) | ||
| .every((field) => !field.show) | ||
| } | ||
| /> | ||
| </td> | ||
| <td className="py-2"> | ||
| <Switch | ||
| checked={field.required} | ||
| onCheckedChange={(required) => { | ||
| updateQuestion(questionIdx, { | ||
| [field.id]: { | ||
| show: field.show, | ||
| required, | ||
| }, | ||
| required: true, | ||
| }); | ||
| }} | ||
| disabled={!field.show} | ||
| /> | ||
| </td> | ||
| </tr> | ||
| ))} |
There was a problem hiding this comment.
Add a unique key prop to items in fields.map
In the fields.map function rendering table rows (lines 142-181), each <tr> element needs a unique key prop. This helps React identify which items have changed, are added, or are removed, preventing potential rendering issues.
Apply this diff to add the key prop:
{fields.map((field) => (
- <tr className="text-slate-900">
+ <tr key={field.id} className="text-slate-900">📝 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.
| <tr className="text-slate-900"> | |
| <td className="py-2 text-sm">{field.label}</td> | |
| <td className="py-"> | |
| <Switch | |
| checked={field.show} | |
| onCheckedChange={(show) => { | |
| updateQuestion(questionIdx, { | |
| [field.id]: { | |
| show, | |
| required: field.required, | |
| }, | |
| // when show changes, and the field is required, the question should be required | |
| ...(show && field.required && { required: true }), | |
| }); | |
| }} | |
| disabled={ | |
| // if all the other fields are hidden, this should be disabled | |
| fields | |
| .filter((currentField) => currentField.id !== field.id) | |
| .every((field) => !field.show) | |
| } | |
| /> | |
| </td> | |
| <td className="py-2"> | |
| <Switch | |
| checked={field.required} | |
| onCheckedChange={(required) => { | |
| updateQuestion(questionIdx, { | |
| [field.id]: { | |
| show: field.show, | |
| required, | |
| }, | |
| required: true, | |
| }); | |
| }} | |
| disabled={!field.show} | |
| /> | |
| </td> | |
| </tr> | |
| ))} | |
| {fields.map((field) => ( | |
| <tr key={field.id} className="text-slate-900"> | |
| <td className="py-2 text-sm">{field.label}</td> | |
| <td className="py-"> | |
| <Switch | |
| checked={field.show} | |
| onCheckedChange={(show) => { | |
| updateQuestion(questionIdx, { | |
| [field.id]: { | |
| show, | |
| required: field.required, | |
| }, | |
| // when show changes, and the field is required, the question should be required | |
| ...(show && field.required && { required: true }), | |
| }); | |
| }} | |
| disabled={ | |
| // if all the other fields are hidden, this should be disabled | |
| fields | |
| .filter((currentField) => currentField.id !== field.id) | |
| .every((field) => !field.show) | |
| } | |
| /> | |
| </td> | |
| <td className="py-2"> | |
| <Switch | |
| checked={field.required} | |
| onCheckedChange={(required) => { | |
| updateQuestion(questionIdx, { | |
| [field.id]: { | |
| show: field.show, | |
| required, | |
| }, | |
| required: true, | |
| }); | |
| }} | |
| disabled={!field.show} | |
| /> | |
| </td> | |
| </tr> | |
| ))} |
🧰 Tools
Biome
[error] 142-142: Missing key property for this element in iterable.
The order of the items may change, and having a key can help React identify which item was moved.
Check the React documentation.(lint/correctness/useJsxKeyInIterable)
| if (question.type === TSurveyQuestionTypeEnum.Address) { | ||
| return [ | ||
| question.isAddressLine1Required, | ||
| question.isAddressLine2Required, | ||
| question.isCityRequired, | ||
| question.isCountryRequired, | ||
| question.isStateRequired, | ||
| question.isZipRequired, | ||
| ].some((condition) => condition === true); | ||
| question.addressLine1, | ||
| question.addressLine2, | ||
| question.city, | ||
| question.state, | ||
| question.zip, | ||
| question.country, | ||
| ] | ||
| .filter((field) => field.show) | ||
| .some((condition) => condition.required === true); | ||
| } | ||
|
|
||
| if (question.type === TSurveyQuestionTypeEnum.ContactInfo) { | ||
| return [question.firstName, question.lastName, question.email, question.phone, question.company] | ||
| .filter((field) => field.show) | ||
| .some((condition) => condition.required === true); | ||
| } | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor duplicated logic in getIsRequiredToggleDisabled function
The code for determining if the required toggle should be disabled is similar for both Address and ContactInfo question types. Consider refactoring this logic to reduce duplication and improve maintainability.
Here's a possible refactored version:
if (question.type === TSurveyQuestionTypeEnum.Address || question.type === TSurveyQuestionTypeEnum.ContactInfo) {
+ const fields =
+ question.type === TSurveyQuestionTypeEnum.Address
+ ? [
+ question.addressLine1,
+ question.addressLine2,
+ question.city,
+ question.state,
+ question.zip,
+ question.country,
+ ]
+ : [
+ question.firstName,
+ question.lastName,
+ question.email,
+ question.phone,
+ question.company,
+ ];
+ return fields
+ .filter((field) => field.show)
+ .some((condition) => condition.required === true);
}📝 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.
| if (question.type === TSurveyQuestionTypeEnum.Address) { | |
| return [ | |
| question.isAddressLine1Required, | |
| question.isAddressLine2Required, | |
| question.isCityRequired, | |
| question.isCountryRequired, | |
| question.isStateRequired, | |
| question.isZipRequired, | |
| ].some((condition) => condition === true); | |
| question.addressLine1, | |
| question.addressLine2, | |
| question.city, | |
| question.state, | |
| question.zip, | |
| question.country, | |
| ] | |
| .filter((field) => field.show) | |
| .some((condition) => condition.required === true); | |
| } | |
| if (question.type === TSurveyQuestionTypeEnum.ContactInfo) { | |
| return [question.firstName, question.lastName, question.email, question.phone, question.company] | |
| .filter((field) => field.show) | |
| .some((condition) => condition.required === true); | |
| } | |
| if (question.type === TSurveyQuestionTypeEnum.Address || question.type === TSurveyQuestionTypeEnum.ContactInfo) { | |
| const fields = | |
| question.type === TSurveyQuestionTypeEnum.Address | |
| ? [ | |
| question.addressLine1, | |
| question.addressLine2, | |
| question.city, | |
| question.state, | |
| question.zip, | |
| question.country, | |
| ] | |
| : [ | |
| question.firstName, | |
| question.lastName, | |
| question.email, | |
| question.phone, | |
| question.company, | |
| ]; | |
| return fields | |
| .filter((field) => field.show) | |
| .some((condition) => condition.required === true); | |
| } |
| ) : question.type === TSurveyQuestionTypeEnum.ContactInfo ? ( | ||
| <ContactInfoQuestionForm | ||
| localSurvey={localSurvey} | ||
| question={question} | ||
| questionIdx={questionIdx} | ||
| updateQuestion={updateQuestion} | ||
| lastQuestion={lastQuestion} | ||
| selectedLanguageCode={selectedLanguageCode} | ||
| setSelectedLanguageCode={setSelectedLanguageCode} | ||
| isInvalid={isInvalid} | ||
| attributeClasses={attributeClasses} | ||
| /> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider refactoring the rendering logic for question forms
The rendering logic uses a long chain of conditional statements to render the appropriate question form component based on question.type. Consider refactoring this by using a mapping between TSurveyQuestionTypeEnum values and their corresponding components. This approach enhances scalability and maintainability as new question types are added.
Here's an example of how you might implement this:
+ const questionFormComponents = {
+ [TSurveyQuestionTypeEnum.OpenText]: OpenQuestionForm,
+ [TSurveyQuestionTypeEnum.MultipleChoiceSingle]: MultipleChoiceQuestionForm,
+ [TSurveyQuestionTypeEnum.MultipleChoiceMulti]: MultipleChoiceQuestionForm,
+ [TSurveyQuestionTypeEnum.NPS]: NPSQuestionForm,
+ [TSurveyQuestionTypeEnum.CTA]: CTAQuestionForm,
+ [TSurveyQuestionTypeEnum.Rating]: RatingQuestionForm,
+ [TSurveyQuestionTypeEnum.Consent]: ConsentQuestionForm,
+ [TSurveyQuestionTypeEnum.Date]: DateQuestionForm,
+ [TSurveyQuestionTypeEnum.PictureSelection]: PictureSelectionForm,
+ [TSurveyQuestionTypeEnum.FileUpload]: FileUploadQuestionForm,
+ [TSurveyQuestionTypeEnum.Cal]: CalQuestionForm,
+ [TSurveyQuestionTypeEnum.Matrix]: MatrixQuestionForm,
+ [TSurveyQuestionTypeEnum.Address]: AddressQuestionForm,
+ [TSurveyQuestionTypeEnum.Ranking]: RankingQuestionForm,
+ [TSurveyQuestionTypeEnum.ContactInfo]: ContactInfoQuestionForm,
+ };
+
+ const QuestionFormComponent = questionFormComponents[question.type];
+
+ return (
+ // ... other code
+ {QuestionFormComponent ? (
+ <QuestionFormComponent
+ localSurvey={localSurvey}
+ question={question}
+ questionIdx={questionIdx}
+ updateQuestion={updateQuestion}
+ lastQuestion={lastQuestion}
+ selectedLanguageCode={selectedLanguageCode}
+ setSelectedLanguageCode={setSelectedLanguageCode}
+ isInvalid={isInvalid}
+ attributeClasses={attributeClasses}
+ // Include any additional props as necessary
+ />
+ ) : null}
+ // ... other code
+ );Committable suggestion was skipped due to low confidence.
| export const ZSurveyQuestionSummaryContactInfo = z.object({ | ||
| type: z.literal("contactInfo"), | ||
| question: ZSurveyContactInfoQuestion, | ||
| responseCount: z.number(), | ||
| samples: z.array( | ||
| z.object({ | ||
| id: z.string(), | ||
| updatedAt: z.date(), | ||
| value: z.array(z.string()), | ||
| person: z | ||
| .object({ | ||
| id: ZId, | ||
| userId: z.string(), | ||
| }) | ||
| .nullable(), | ||
| personAttributes: ZAttributes.nullable(), | ||
| }) | ||
| ), | ||
| }); | ||
|
|
||
| export type TSurveyQuestionSummaryContactInfo = z.infer<typeof ZSurveyQuestionSummaryContactInfo>; | ||
|
|
There was a problem hiding this comment.
Consider changing 'value' to an object in 'ZSurveyQuestionSummaryContactInfo'
In ZSurveyQuestionSummaryContactInfo, the value field is currently defined as z.array(z.string()). Since ContactInfo responses contain multiple named fields (firstName, lastName, email, phone, company), consider defining value as an object mapping field names to their respective values for better clarity and data integrity.
Apply this diff to change value to an object:
export const ZSurveyQuestionSummaryContactInfo = z.object({
...
- value: z.array(z.string()),
+ value: z.object({
+ firstName: z.string().optional(),
+ lastName: z.string().optional(),
+ email: z.string().optional(),
+ phone: z.string().optional(),
+ company: z.string().optional(),
+ }),
...
});📝 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 ZSurveyQuestionSummaryContactInfo = z.object({ | |
| type: z.literal("contactInfo"), | |
| question: ZSurveyContactInfoQuestion, | |
| responseCount: z.number(), | |
| samples: z.array( | |
| z.object({ | |
| id: z.string(), | |
| updatedAt: z.date(), | |
| value: z.array(z.string()), | |
| person: z | |
| .object({ | |
| id: ZId, | |
| userId: z.string(), | |
| }) | |
| .nullable(), | |
| personAttributes: ZAttributes.nullable(), | |
| }) | |
| ), | |
| }); | |
| export type TSurveyQuestionSummaryContactInfo = z.infer<typeof ZSurveyQuestionSummaryContactInfo>; | |
| export const ZSurveyQuestionSummaryContactInfo = z.object({ | |
| type: z.literal("contactInfo"), | |
| question: ZSurveyContactInfoQuestion, | |
| responseCount: z.number(), | |
| samples: z.array( | |
| z.object({ | |
| id: z.string(), | |
| updatedAt: z.date(), | |
| value: z.object({ | |
| firstName: z.string().optional(), | |
| lastName: z.string().optional(), | |
| email: z.string().optional(), | |
| phone: z.string().optional(), | |
| company: z.string().optional(), | |
| }), | |
| person: z | |
| .object({ | |
| id: ZId, | |
| userId: z.string(), | |
| }) | |
| .nullable(), | |
| personAttributes: ZAttributes.nullable(), | |
| }) | |
| ), | |
| }); | |
| export type TSurveyQuestionSummaryContactInfo = z.infer<typeof ZSurveyQuestionSummaryContactInfo>; |
There was a problem hiding this comment.
Hey @pandeymangg , new Contact question looks really good along with updated Ui for address question 🚀🙌🏻
Here are some issues that i noticed
- Validation missing when creating survey from API. I was able to create a question with all fields hidden and it is not possible to change it from UI then
Settingslabel looks a bit odd (UI feels messy). do we really need this ?
- Contact Data, show and required labels are too much bold, maybe we can set them to semi bold
- Contact/ Address question does not consider input color
Screen-Recording-2024-09-27-at-9.54.48.AM.mp4
- Question headline has some extra spacing on left
- Number field in contact info also takes text (validation missing)
- Too much spacing in y direction for input fields (check older address question)
- Adding an image breaks the question card
Screen-Recording-2024-09-27-at-10.03.18.AM.mp4
- Contact info card missing from filters
- Add some spacing around description
-
Fix failing e2e test due to changes in address question input and also add e2e test for contact info question
-
Address feedback from codeRabbit
Rest looks good 🚀
| <EmailFooter /> | ||
| </EmailTemplateWrapper> | ||
| ); | ||
| case TSurveyQuestionTypeEnum.ContactInfo: |
There was a problem hiding this comment.
Since address and contact info, both have the exact same UI with only difference in labels, can we combine them in a single use case ?
packages/lib/response/utils.ts
Outdated
|
|
||
| break; | ||
| } | ||
| case TSurveyQuestionTypeEnum.ContactInfo: { |
There was a problem hiding this comment.
Same here, exact same code as address, so can be combined to avoid repetition 😊
| className="fb-py-3" | ||
| type={field.id === "email" ? "email" : "text"} | ||
| // @ts-expect-error | ||
| onChange={(e) => handleChange(field.id, e?.target?.value ?? "")} |
There was a problem hiding this comment.
target can be changed to currentTarget to avoid type error here, so we don't need @ts-expect-error
onChange={(e) => handleChange(field.id, e.currentTarget.value ?? "")}
| className="fb-py-3" | ||
| type={field.id === "email" ? "email" : "text"} | ||
| // @ts-expect-error | ||
| onChange={(e) => handleChange(field.id, e?.target?.value ?? "")} |
| return <ArrayResponse value={responseData} />; | ||
| } | ||
| break; | ||
| case TSurveyQuestionTypeEnum.ContactInfo: |
There was a problem hiding this comment.
Can be combined like we do for multi select
| childBorder | ||
| customContainerClass="p-0 mt-4"></AdvancedOptionToggle> | ||
|
|
||
| <table className="mt-4 w-1/2 table-fixed"> |
There was a problem hiding this comment.
We create a reusable component for this settings table, which can be used for both contact and address question 😊
| return ( | ||
| <input | ||
| className={cn( | ||
| "focus:fb-border-brand fb-flex fb-w-full fb-border fb-border-border fb-bg-transparent fb-rounded-custom fb-px-3 fb-py-2 fb-text-sm fb-text-subheading placeholder:fb-text-placeholder focus:fb-outline-none focus:fb-ring-2 focus:fb-ring-offset-2 disabled:fb-cursor-not-allowed disabled:fb-opacity-50 dark:fb-border-slate-500 dark:fb-text-slate-300", |
There was a problem hiding this comment.
We also need to add dir="auto" here
|
|
||
| await Promise.all(updationPromises); | ||
|
|
||
| console.log("Data migration completed"); |
There was a problem hiding this comment.
There is already a log for data migration completed below. Instead we can replace this with, number of surveys transformed
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (4)
apps/web/app/(app)/(survey-editor)/environments/[environmentId]/surveys/[surveyId]/edit/components/AddressQuestionForm.tsx (2)
36-67: LGTM: Centralized field configuration improves code organization.The new
fieldsarray effectively centralizes the configuration of address fields, enhancing code maintainability and allowing for more dynamic field handling.Consider extracting this
fieldsarray to a separate constant outside the component to improve performance and reduce unnecessary re-creation on each render. For example:const ADDRESS_FIELDS = [ { id: "addressLine1", label: "Address Line 1" }, { id: "addressLine2", label: "Address Line 2" }, { id: "city", label: "City" }, { id: "state", label: "State" }, { id: "zip", label: "Zip" }, { id: "country", label: "Country" }, ]; // In the component: const fields = ADDRESS_FIELDS.map(field => ({ ...field, ...question[field.id as keyof TSurveyAddressQuestion], }));This approach separates the static configuration from the dynamic data, potentially improving performance and readability.
Line range hint
1-170: Great improvements to address field management and UI.The changes made to this component significantly enhance its maintainability, flexibility, and user interface. The introduction of a centralized
fieldsarray and theQuestionToggleTablecomponent makes the code more scalable and easier to manage.To further improve the code:
Consider adding a brief comment or documentation explaining the new structure and its benefits. This will help other developers understand the design decisions and how to work with this component effectively.
If not already present, it might be helpful to add unit tests for the new logic, especially the
useEffecthook that determines if all fields are optional.Ensure that the
QuestionToggleTablecomponent is well-documented, particularly regarding its props and expected behavior.These additional steps will make the code even more robust and easier for the team to maintain in the long run.
apps/web/app/(app)/environments/[environmentId]/surveys/[surveyId]/(analysis)/responses/components/ResponseTableColumns.tsx (2)
38-53: LGTM with a minor suggestion.The
getContactInfoFieldLabelfunction is well-implemented and consistent with the existing codebase. It effectively maps contact info fields to their corresponding labels.Consider adding a default return value to handle unexpected inputs:
const getContactInfoFieldLabel = (field: string) => { switch (field) { case "firstName": return "First Name"; case "lastName": return "Last Name"; case "email": return "Email"; case "phone": return "Phone"; case "company": return "Company"; default: - break; + return field; // or return a default label like "Unknown Field" } };
108-131: LGTM with suggestions for improvement.The implementation for the new "contactInfo" question type is well-structured and consistent with other question types in the function.
Consider the following improvements:
- Address the static analysis warning by wrapping the case in a block:
case "contactInfo": { const contactInfoFields = ["firstName", "lastName", "email", "phone", "company"]; return contactInfoFields.map((contactInfoField) => { // ... rest of the implementation }); }
- To improve flexibility, consider defining the
contactInfoFieldsarray outside the function or fetching it from a configuration:const CONTACT_INFO_FIELDS = ["firstName", "lastName", "email", "phone", "company"]; // In the switch statement case "contactInfo": { return CONTACT_INFO_FIELDS.map((contactInfoField) => { // ... rest of the implementation }); }This approach would make it easier to modify the fields in the future without changing the function implementation.
🧰 Tools
🪛 Biome
[error] 109-109: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.The declaration is defined in this switch clause:
Unsafe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (10)
- apps/web/app/(app)/(survey-editor)/environments/[environmentId]/surveys/[surveyId]/edit/components/AddressQuestionForm.tsx (3 hunks)
- apps/web/app/(app)/(survey-editor)/environments/[environmentId]/surveys/[surveyId]/edit/components/ContactInfoQuestionForm.tsx (1 hunks)
- apps/web/app/(app)/(survey-editor)/environments/[environmentId]/surveys/[surveyId]/edit/components/LogicEditor.tsx (1 hunks)
- apps/web/app/(app)/(survey-editor)/environments/[environmentId]/surveys/[surveyId]/edit/components/QuestionCard.tsx (3 hunks)
- apps/web/app/(app)/(survey-editor)/environments/[environmentId]/surveys/[surveyId]/edit/components/SurveyEditor.tsx (1 hunks)
- apps/web/app/(app)/environments/[environmentId]/surveys/[surveyId]/(analysis)/responses/components/ResponseTableColumns.tsx (2 hunks)
- apps/web/app/(app)/environments/[environmentId]/surveys/[surveyId]/(analysis)/summary/components/AddressSummary.tsx (3 hunks)
- apps/web/app/(app)/environments/[environmentId]/surveys/[surveyId]/(analysis)/summary/components/ContactInfoSummary.tsx (1 hunks)
- apps/web/app/(app)/environments/[environmentId]/surveys/[surveyId]/(analysis)/summary/components/SummaryList.tsx (2 hunks)
- apps/web/app/lib/surveys/surveys.ts (3 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- apps/web/app/(app)/(survey-editor)/environments/[environmentId]/surveys/[surveyId]/edit/components/ContactInfoQuestionForm.tsx
- apps/web/app/(app)/(survey-editor)/environments/[environmentId]/surveys/[surveyId]/edit/components/QuestionCard.tsx
- apps/web/app/(app)/environments/[environmentId]/surveys/[surveyId]/(analysis)/summary/components/AddressSummary.tsx
- apps/web/app/(app)/environments/[environmentId]/surveys/[surveyId]/(analysis)/summary/components/ContactInfoSummary.tsx
- apps/web/app/(app)/environments/[environmentId]/surveys/[surveyId]/(analysis)/summary/components/SummaryList.tsx
🧰 Additional context used
🪛 Biome
apps/web/app/(app)/environments/[environmentId]/surveys/[surveyId]/(analysis)/responses/components/ResponseTableColumns.tsx
[error] 109-109: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.The declaration is defined in this switch clause:
Unsafe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
🔇 Additional comments (14)
apps/web/app/(app)/(survey-editor)/environments/[environmentId]/surveys/[surveyId]/edit/components/AddressQuestionForm.tsx (1)
4-4: LGTM: New imports added for enhanced functionality.The addition of
useEffectandQuestionToggleTableimports aligns with the changes made in the component, supporting new side-effect handling and improved UI for managing question toggles.Also applies to: 10-10
apps/web/app/(app)/(survey-editor)/environments/[environmentId]/surveys/[surveyId]/edit/components/SurveyEditor.tsx (5)
136-149: LGTM: SurveyMenuBar component updated correctlyThe SurveyMenuBar component has been updated to include the new
isCxModeprop, which is consistent with the PR objectives. All other props are correctly maintained, ensuring the component's functionality remains intact.
154-159: LGTM: QuestionsAudienceTabs component updated correctlyThe QuestionsAudienceTabs component has been updated to include the new
isCxModeprop, maintaining consistency with the PR objectives and the changes in SurveyMenuBar. All other props are correctly maintained, ensuring the component's functionality remains intact.
161-178: LGTM: QuestionsView component updated and conditionally renderedThe QuestionsView component has been updated to include the new
isCxModeprop, maintaining consistency with the PR objectives and the changes in other components. The component is now conditionally rendered based on theactiveViewstate, which improves the overall structure of the component. All other props are correctly maintained, ensuring the component's functionality remains intact.
180-224: LGTM: StylingView and PreviewSurvey components updated correctlyThe StylingView component has been updated to include the new
isCxModeprop, maintaining consistency with the PR objectives and the changes in other components. It is also conditionally rendered based on theactiveViewstate andproduct.styling.allowStyleOverwrite, which improves the overall structure of the component.The PreviewSurvey component has had its props reordered, which doesn't affect functionality but improves code readability.
All other props for both components are correctly maintained, ensuring their functionality remains intact.
135-226: Summary: SurveyEditor component successfully updatedThe changes to the SurveyEditor component are well-implemented and align with the PR objectives to add a new question type. Key points:
- The new
isCxModeprop has been consistently added to relevant child components (SurveyMenuBar, QuestionsAudienceTabs, QuestionsView, and StylingView).- Conditional rendering has been improved for QuestionsView and StylingView components.
- Prop ordering in the PreviewSurvey component has been optimized for better readability.
- Overall structure and functionality of the SurveyEditor component remain intact.
These changes enhance the component's flexibility to handle the new question type while maintaining its existing functionality.
apps/web/app/(app)/environments/[environmentId]/surveys/[surveyId]/(analysis)/responses/components/ResponseTableColumns.tsx (1)
Line range hint
1-385: Overall, the implementation looks good and aligns with the PR objectives.The new "contactInfo" question type has been successfully implemented in a manner consistent with existing code patterns. The changes are well-integrated and should provide the desired functionality for collecting contact information in surveys.
A few minor suggestions were provided to enhance code robustness and flexibility. Once these are addressed, the code will be ready for merging.
🧰 Tools
🪛 Biome
[error] 109-109: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.The declaration is defined in this switch clause:
Unsafe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
apps/web/app/(app)/(survey-editor)/environments/[environmentId]/surveys/[surveyId]/edit/components/LogicEditor.tsx (3)
Line range hint
1-590: Summary: New contact info question type integration looks goodThe addition of the "contactInfo" condition to the
conditionsobject is the only change in this file. This modification aligns well with the existing structure and should seamlessly integrate with the current implementation of theLogicEditorcomponent. The component's design appears to handle different question types generically, which should accommodate this new type without requiring further changes in this file.To ensure a smooth integration across the entire codebase:
- Verify that all necessary type definitions and interfaces have been updated.
- Check if any other components or utilities that interact with survey questions need to be aware of this new type.
- Consider adding or updating unit tests to cover the new "contactInfo" question type in the logic editor.
Line range hint
1-590: Verify type definitions and interfacesThe addition of the "contactInfo" condition should work seamlessly with the existing implementation. However, to ensure type safety and maintain consistency, please verify the following:
- Check if the
TSurveyQuestionTypeEnumin the types file includes the new "contactInfo" type.- Verify if any interfaces or types related to survey questions or logic conditions need to be updated to include the new "contactInfo" type.
Run the following script to check for necessary type updates:
#!/bin/bash # Description: Check for necessary type updates related to "contactInfo" # Test 1: Verify TSurveyQuestionTypeEnum echo "Checking TSurveyQuestionTypeEnum for 'contactInfo':" rg --type typescript "TSurveyQuestionTypeEnum.*contactInfo" packages/types # Test 2: Check other relevant interfaces and types echo "Checking other relevant interfaces and types:" rg --type typescript "interface.*contactInfo|type.*contactInfo" packages/types
90-90: LGTM! Verify integration of new condition.The addition of the "contactInfo" condition is consistent with the existing structure and includes appropriate values. This change supports the new contact info question type mentioned in the PR objectives.
To ensure proper integration, please run the following script to check if the new condition is handled correctly throughout the component:
apps/web/app/lib/surveys/surveys.ts (4)
35-35: Inclusion ofcontactInfoinconditionOptionsis appropriateAdding
contactInfowith the condition["is"]ensures that the new question type is correctly integrated into the filtering conditions, consistent with other similar question types likeaddressandranking.
46-46: Addition ofcontactInfotofilterOptionsenhances filtering capabilitiesIncluding
contactInfowith options["Filled out", "Skipped"]allows for proper filtering based on user interactions with theContactInfoquestion type, aligning it with existing question types likeaddressandranking.
278-279: HandlingContactInfoingetFormattedFiltersfunctionBy adding
TSurveyQuestionTypeEnum.ContactInfoto the case alongsideOpenTextandAddress, the filtering logic now appropriately processes responses for theContactInfoquestion type.
282-282:⚠️ Potential issuePotential impact of changing
opto"filledOut"for existing question typesChanging the operation from
"submitted"to"filledOut"forOpenTextandAddressquestion types may affect existing filtering functionality that relies on"submitted". Please verify that this change does not introduce regressions or alter the expected behavior of existing filters.Run the following script to check for any occurrences where
op: "submitted"is used withOpenTextorAddressquestion types:✅ Verification successful
Verification Passed: No usage of
op: "submitted"found forOpenTextandAddressquestion types.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Find code that uses 'op: "submitted"' for 'OpenText' and 'Address' question types. rg --type js 'case\s+TSurveyQuestionTypeEnum\.(OpenText|Address):(\n|.)*?op:\s*"submitted"' -A 5Length of output: 290
Script:
#!/bin/bash # Description: Find code that uses 'op: "submitted"' for 'OpenText' and 'Address' question types with multiline support. rg -U --type js 'case\s+TSurveyQuestionTypeEnum\.(OpenText|Address):(\n|.)*?op:\s*"submitted"' -A 5Length of output: 101
| <QuestionToggleTable | ||
| fields={fields} | ||
| onShowToggle={(field, show) => { | ||
| updateQuestion(questionIdx, { | ||
| isAddressLine1Required: !question.isAddressLine1Required, | ||
| required: true, | ||
| }) | ||
| } | ||
| htmlId="isAddressRequired" | ||
| title="Required: Address Line 1" | ||
| description="" | ||
| childBorder | ||
| customContainerClass="p-0 mt-4"></AdvancedOptionToggle> | ||
| <AdvancedOptionToggle | ||
| isChecked={question.isAddressLine2Required} | ||
| onToggle={() => | ||
| [field.id]: { | ||
| show, | ||
| required: field.required, | ||
| }, | ||
| // when show changes, and the field is required, the question should be required | ||
| ...(show && field.required && { required: true }), | ||
| }); | ||
| }} | ||
| onRequiredToggle={(field, required) => { | ||
| updateQuestion(questionIdx, { | ||
| isAddressLine2Required: !question.isAddressLine2Required, | ||
| [field.id]: { | ||
| show: field.show, | ||
| required, | ||
| }, | ||
| required: true, | ||
| }) | ||
| } | ||
| htmlId="isAddressLine2Required" | ||
| title="Required: Address Line 2" | ||
| description="" | ||
| childBorder | ||
| customContainerClass="p-0 mt-4"></AdvancedOptionToggle> | ||
| <AdvancedOptionToggle | ||
| isChecked={question.isCityRequired} | ||
| onToggle={() => | ||
| updateQuestion(questionIdx, { isCityRequired: !question.isCityRequired, required: true }) | ||
| } | ||
| htmlId="isCityRequired" | ||
| title="Required: City / Town" | ||
| description="" | ||
| childBorder | ||
| customContainerClass="p-0 mt-4"></AdvancedOptionToggle> | ||
| <AdvancedOptionToggle | ||
| isChecked={question.isStateRequired} | ||
| onToggle={() => | ||
| updateQuestion(questionIdx, { isStateRequired: !question.isStateRequired, required: true }) | ||
| } | ||
| htmlId="isStateRequired" | ||
| title="Required: State / Region" | ||
| description="" | ||
| childBorder | ||
| customContainerClass="p-0 mt-4"></AdvancedOptionToggle> | ||
| <AdvancedOptionToggle | ||
| isChecked={question.isZipRequired} | ||
| onToggle={() => | ||
| updateQuestion(questionIdx, { isZipRequired: !question.isZipRequired, required: true }) | ||
| } | ||
| htmlId="isZipRequired" | ||
| title="Required: ZIP / Post Code" | ||
| description="" | ||
| childBorder | ||
| customContainerClass="p-0 mt-4"></AdvancedOptionToggle> | ||
| <AdvancedOptionToggle | ||
| isChecked={question.isCountryRequired} | ||
| onToggle={() => | ||
| updateQuestion(questionIdx, { isCountryRequired: !question.isCountryRequired, required: true }) | ||
| } | ||
| htmlId="iscountryRequired" | ||
| title="Required: Country" | ||
| description="" | ||
| childBorder | ||
| customContainerClass="p-0 mt-4"></AdvancedOptionToggle> | ||
| }); | ||
| }} | ||
| /> |
There was a problem hiding this comment.
LGTM: QuestionToggleTable improves UI for managing address fields.
The introduction of the QuestionToggleTable component significantly improves the UI for managing address field visibility and requirements. The callbacks are well-structured and handle both individual field updates and the overall question required status.
However, there's a potential issue with the key prop in the mapping of fields, as noted in a previous review comment. To resolve this, ensure that a unique key prop is provided when rendering the list items within the QuestionToggleTable component. If this isn't already handled within the component, consider passing the id of each field as a key:
<QuestionToggleTable
fields={fields}
keyExtractor={(field) => field.id}
// ... other props
/>This change will help React efficiently update the list and avoid potential issues with component state.
| useEffect(() => { | ||
| const allFieldsAreOptional = [ | ||
| question.addressLine1, | ||
| question.addressLine2, | ||
| question.city, | ||
| question.state, | ||
| question.zip, | ||
| question.country, | ||
| ] | ||
| .filter((field) => field.show) | ||
| .every((field) => !field.required); | ||
|
|
||
| if (allFieldsAreOptional) { | ||
| updateQuestion(questionIdx, { required: false }); | ||
| } | ||
| // eslint-disable-next-line react-hooks/exhaustive-deps | ||
| }, [ | ||
| question.addressLine1, | ||
| question.addressLine2, | ||
| question.city, | ||
| question.state, | ||
| question.zip, | ||
| question.country, | ||
| ]); |
There was a problem hiding this comment.
Consider including all dependencies in the useEffect hook.
The useEffect hook correctly checks if all visible address fields are optional and updates the question's required status. However, the ESLint rule for exhaustive dependencies is disabled, which may lead to potential bugs.
To address this, consider including all necessary dependencies in the dependency array:
useEffect(() => {
// ... existing code ...
}, [
question.addressLine1,
question.addressLine2,
question.city,
question.state,
question.zip,
question.country,
+ updateQuestion,
+ questionIdx
]);This change ensures that the effect runs when updateQuestion or questionIdx change, preventing potential stale closure issues. After making this change, you can remove the ESLint disable comment.
📝 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.
| useEffect(() => { | |
| const allFieldsAreOptional = [ | |
| question.addressLine1, | |
| question.addressLine2, | |
| question.city, | |
| question.state, | |
| question.zip, | |
| question.country, | |
| ] | |
| .filter((field) => field.show) | |
| .every((field) => !field.required); | |
| if (allFieldsAreOptional) { | |
| updateQuestion(questionIdx, { required: false }); | |
| } | |
| // eslint-disable-next-line react-hooks/exhaustive-deps | |
| }, [ | |
| question.addressLine1, | |
| question.addressLine2, | |
| question.city, | |
| question.state, | |
| question.zip, | |
| question.country, | |
| ]); | |
| useEffect(() => { | |
| const allFieldsAreOptional = [ | |
| question.addressLine1, | |
| question.addressLine2, | |
| question.city, | |
| question.state, | |
| question.zip, | |
| question.country, | |
| ] | |
| .filter((field) => field.show) | |
| .every((field) => !field.required); | |
| if (allFieldsAreOptional) { | |
| updateQuestion(questionIdx, { required: false }); | |
| } | |
| }, [ | |
| question.addressLine1, | |
| question.addressLine2, | |
| question.city, | |
| question.state, | |
| question.zip, | |
| question.country, | |
| updateQuestion, | |
| questionIdx | |
| ]); |
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Outside diff range and nitpick comments (4)
packages/surveys/src/components/questions/AddressQuestion.tsx (1)
50-80: LGTM: Well-structured fields array.The new
fieldsarray centralizes field configurations, making the code more maintainable and extensible. The use of the spread operator allows for flexible field configurations.Consider extracting this array to a separate constant or useMemo hook to prevent unnecessary re-creation on each render:
const fields = useMemo(() => [ { id: "addressLine1", placeholder: "Address Line 1", ...question.addressLine1, }, // ... other fields ], [question]);This optimization could be beneficial, especially if the component re-renders frequently.
apps/web/playwright/utils/mock.ts (2)
160-163: LGTM! Consider using a more generic placeholder.The addition of the
contactInfofield aligns well with the PR objective of introducing a new contact information question type. The structure is consistent with other question types in the mock data.Consider using a more generic placeholder, such as "Enter your name" or "Full name", instead of "First Name". This would make the field more flexible for different use cases.
Line range hint
228-230: LGTM! Consider improving German translations.The additions of
dateQuestion,calQuestion, andbuttonLabelin thethankYouCardare good improvements to the mock data, aligning with new feature support and providing a call-to-action.The German translations for
dateQuestionandcalQuestioncould be improved:
- Change "Dies ist date question" to "Dies ist eine Datumsfrage"
- Change "Dies ist cal question" to "Dies ist eine Kalenderfrage"
This would make the translations more accurate and consistent with the rest of the German content.
Also applies to: 249-251
apps/web/playwright/survey.spec.ts (1)
183-187: LGTM! Consider adding an assertion for the filled value.The implementation of the Contact Info Question test is consistent with other question types and follows the established pattern. Good job on maintaining consistency!
Consider adding an assertion to verify that the filled value is correct:
await page.getByPlaceholder(surveys.createAndSubmit.contactInfo.placeholder).fill("John Doe"); +await expect(page.getByPlaceholder(surveys.createAndSubmit.contactInfo.placeholder)).toHaveValue("John Doe"); await page.locator("#questionCard-11").getByRole("button", { name: "Next" }).click();This additional assertion would ensure that the input field correctly retains the entered value before moving to the next question.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
- apps/web/playwright/survey.spec.ts (1 hunks)
- apps/web/playwright/utils/helper.ts (1 hunks)
- apps/web/playwright/utils/mock.ts (1 hunks)
- packages/surveys/src/components/questions/AddressQuestion.tsx (3 hunks)
- packages/surveys/src/components/questions/ContactInfoQuestion.tsx (1 hunks)
🔇 Additional comments (13)
packages/surveys/src/components/questions/AddressQuestion.tsx (7)
4-4: LGTM: Improved imports and component interface.The addition of the
Inputcomponent import and theuseMemohook suggests a move towards more modular and optimized code. The simplification of theAddressQuestionPropsinterface by removingautoFocusandautoFocusEnabledprops is a good step towards a cleaner component API.Also applies to: 9-9
83-92: LGTM: Efficient state update in handleChange.The
handleChangefunction is well-implemented. It efficiently updates only the changed field while preserving other values, and it correctly handles cases where some fields might not be shown. The use ofmapensures immutability in the state update, which is a good practice in React.
94-104: LGTM: Robust submission handling.The
handleSubmitfunction is well-implemented. It correctly updates the time-to-completion (ttc) metric and handles the edge case of all empty strings by submitting an empty array. This approach ensures that no unnecessary data is submitted while still capturing valid responses.
123-137: LGTM: Clear and comprehensive field requirement logic.The
isFieldRequiredfunction is well-implemented. It correctly handles different scenarios for field requirement, considering both individual field settings and the overall question requirement. The logic is clear and easy to understand, which enhances maintainability.
122-152: LGTM: Clean and efficient input field rendering.The rendering of input fields is well-implemented. The use of the
Inputcomponent promotes consistency and reusability, and the conditional rendering based on the 'show' property allows for flexible field display.Regarding the past review comment about changing
targettocurrentTarget, I can see that this suggestion has already been implemented on line 148:onChange={(e) => handleChange(field.id, e?.currentTarget?.value ?? "")}This change correctly addresses the type error without needing
@ts-expect-error. Good job on implementing this improvement!
155-165: LGTM: Proper handling of back navigation.The back button logic is well-implemented. It correctly updates the time-to-completion (ttc) metric before navigating back, ensuring that the time spent on the current question is accurately recorded. The use of
getUpdatedTtcensures consistent ttc updates across the component.
Line range hint
1-177: Overall, excellent refactoring and improvements.The changes to the
AddressQuestioncomponent have significantly improved its structure, efficiency, and maintainability. Key improvements include:
- More modular code structure with the introduction of the
Inputcomponent.- Centralized field configuration for easier management.
- Efficient state updates in the
handleChangefunction.- Robust submission handling with proper edge case management.
- Clear and comprehensive field requirement logic.
- Clean and efficient input field rendering.
- Proper handling of back navigation with accurate ttc updates.
These changes have resulted in a more flexible and performant component. The code is well-structured and follows good React practices. Great job on this refactoring!
packages/surveys/src/components/questions/ContactInfoQuestion.tsx (3)
1-27: LGTM: Imports and interface declaration are well-structured.The imports cover all necessary components and utilities, and the
ContactInfoQuestionPropsinterface is comprehensive and well-typed. This provides a solid foundation for the component.
102-179: LGTM: Well-structured JSX with good use of components.The component's render structure is well-organized, making good use of reusable components and conditional rendering. The dynamic rendering of input fields based on the
fieldsarray is a clean approach to handling variable form fields.
1-181: Overall, well-implemented component with minor optimization opportunities.The
ContactInfoQuestioncomponent is well-structured and effectively handles its responsibilities of rendering and managing a complex form for collecting contact information. It follows React best practices and makes good use of reusable components.Some minor suggestions for further improvement:
- Consider adding error handling for the email and phone inputs to provide better user feedback.
- The
autoFocusprop is defined in the interface but not used in the component. Either implement it or remove it from the interface.- Add comments to explain complex logic, especially around the required field calculations.
Great job on implementing this feature! These suggestions are for fine-tuning and don't detract from the overall quality of the implementation.
apps/web/playwright/utils/mock.ts (1)
Line range hint
1-359: Overall, the changes look good and align with the PR objectives.The additions to the mock data in
apps/web/playwright/utils/mock.tssuccessfully support the new contact info question type and provide additional coverage for date and calendar questions. The German translations have been updated accordingly, maintaining consistency throughout the file.A few minor suggestions were made to improve the placeholder text and German translations, but these are not critical issues. The changes are well-structured and maintain the existing code conventions.
apps/web/playwright/survey.spec.ts (1)
183-187: Verify test setup for the new Contact Info question typeThe new Contact Info question has been seamlessly integrated into the test flow. To ensure comprehensive testing:
- Verify that the
surveys.createAndSubmit.contactInfoobject is properly initialized in the test setup.- Consider adding a verification step to check if the submitted contact info is correctly stored or processed.
Run the following script to check the test setup:
apps/web/playwright/utils/helper.ts (1)
289-293: Inconsistent use of selectors when interacting with form fieldsIn the code for filling the Address question, there's inconsistent use of selectors when toggling fields. For "Address Line 2", "State", and "Country", you use
getByRole("switch").nth(1).click(), whereas for "City" and "Zip", you usegetByRole("cell").nth(2).click(). This inconsistency may lead to confusion and potential issues if the UI changes.Consider verifying whether the use of different selectors is necessary. If the elements are similar, standardizing the selectors can improve code consistency and maintainability.
| const fields = [ | ||
| { | ||
| id: "firstName", | ||
| placeholder: "First Name", | ||
| ...question.firstName, | ||
| }, | ||
| { | ||
| id: "lastName", | ||
| placeholder: "Last Name", | ||
| ...question.lastName, | ||
| }, | ||
| { | ||
| id: "email", | ||
| placeholder: "Email", | ||
| ...question.email, | ||
| }, | ||
| { | ||
| id: "phone", | ||
| placeholder: "Phone", | ||
| ...question.phone, | ||
| }, | ||
| { | ||
| id: "company", | ||
| placeholder: "Company", | ||
| ...question.company, | ||
| }, | ||
| ]; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider moving the fields array outside the component.
The fields array is currently defined within the component, which means it will be recreated on every render. Since this array is static and doesn't depend on any props or state, it can be moved outside the component to improve performance slightly.
You can refactor it like this:
const fields = [
{
id: "firstName",
placeholder: "First Name",
},
{
id: "lastName",
placeholder: "Last Name",
},
{
id: "email",
placeholder: "Email",
},
{
id: "phone",
placeholder: "Phone",
},
{
id: "company",
placeholder: "Company",
},
];
export const ContactInfoQuestion = ({ question, ...otherProps }: ContactInfoQuestionProps) => {
// ... rest of the component
const fieldsWithQuestionProps = useMemo(() =>
fields.map(field => ({ ...field, ...question[field.id as keyof typeof question] })),
[question]
);
// Use fieldsWithQuestionProps instead of fields in the rest of the component
}This approach ensures that the base fields array is only created once, and the useMemo hook will only recompute the fields with question props when the question prop changes.
| const safeValue = useMemo(() => { | ||
| return Array.isArray(value) ? value : ["", "", "", "", ""]; | ||
| }, [value]); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Reconsider the necessity of useMemo for safeValue.
The useMemo hook is used to memoize the safeValue calculation. However, this might be unnecessary if the value prop is not expected to change frequently. If value is stable, you could simplify this to a regular variable.
Consider refactoring to:
const safeValue = Array.isArray(value) ? value : Array(fields.length).fill("");Only keep the useMemo if you've identified that value changes frequently and this calculation is a performance bottleneck.
| const handleChange = (fieldId: string, fieldValue: string) => { | ||
| const newValue = fields.map((field) => { | ||
| if (field.id === fieldId) { | ||
| return fieldValue; | ||
| } | ||
| const existingValue = safeValue?.[fields.findIndex((f) => f.id === field.id)] || ""; | ||
| return field.show ? existingValue : ""; | ||
| }); | ||
| onChange({ [question.id]: newValue }); | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Optimize handleChange function for better performance.
The current implementation of handleChange iterates over all fields for every change, which can be inefficient. We can optimize this by directly updating the specific index.
Consider refactoring handleChange as follows:
const handleChange = (fieldId: string, fieldValue: string) => {
const fieldIndex = fields.findIndex((field) => field.id === fieldId);
if (fieldIndex !== -1) {
const newValue = [...(safeValue || [])];
newValue[fieldIndex] = fieldValue;
onChange({ [question.id]: newValue });
}
};This approach directly updates the changed field without iterating over all fields, improving performance especially for forms with many fields.
| const handleSubmit = (e: Event) => { | ||
| e.preventDefault(); | ||
| const updatedTtc = getUpdatedTtc(ttc, question.id, performance.now() - startTime); | ||
| setTtc(updatedTtc); | ||
| const containsAllEmptyStrings = safeValue?.length === 5 && safeValue.every((item) => item.trim() === ""); | ||
| if (containsAllEmptyStrings) { | ||
| onSubmit({ [question.id]: [] }, updatedTtc); | ||
| } else { | ||
| onSubmit({ [question.id]: safeValue ?? [] }, updatedTtc); | ||
| } | ||
| }; |
There was a problem hiding this comment.
Ensure safeValue is defined before using in handleSubmit.
In the handleSubmit function, safeValue is used without checking if it's defined. This could lead to runtime errors if safeValue is undefined.
Update the handleSubmit function to ensure safeValue is defined:
const handleSubmit = (e: Event) => {
e.preventDefault();
const updatedTtc = getUpdatedTtc(ttc, question.id, performance.now() - startTime);
setTtc(updatedTtc);
const currentValue = safeValue || [];
const containsAllEmptyStrings = currentValue.length === fields.length && currentValue.every((item) => item.trim() === "");
onSubmit({ [question.id]: containsAllEmptyStrings ? [] : currentValue }, updatedTtc);
};This change ensures that we always have a valid array to work with, preventing potential undefined errors.
| const isFieldRequired = () => { | ||
| if (field.required) { | ||
| return true; | ||
| } | ||
|
|
||
| // if all fields are optional and the question is required, then the fields should be required | ||
| if ( | ||
| fields.filter((field) => field.show).every((field) => !field.required) && | ||
| question.required | ||
| ) { | ||
| return true; | ||
| } | ||
|
|
||
| return false; | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Optimize isFieldRequired function.
The isFieldRequired function is defined within the map function, which means it's recreated for each field on every render. This can be inefficient, especially for forms with many fields.
Consider moving this function outside of the render and memoizing it:
const isFieldRequired = useMemo(() => {
const allFieldsOptional = fields.filter((field) => field.show).every((field) => !field.required);
return (fieldId: string) => {
const field = fields.find((f) => f.id === fieldId);
return field?.required || (allFieldsOptional && question.required);
};
}, [fields, question.required]);
// In the JSX
required={isFieldRequired(field.id)}This approach ensures that the function is only recalculated when fields or question.required changes, potentially improving render performance.
| await page.getByRole("row", { name: "Address Line 2" }).getByRole("switch").nth(1).click(); | ||
| await page.getByRole("row", { name: "City" }).getByRole("cell").nth(2).click(); | ||
| await page.getByRole("row", { name: "State" }).getByRole("switch").nth(1).click(); | ||
| await page.getByRole("row", { name: "Zip" }).getByRole("cell").nth(2).click(); | ||
| await page.getByRole("row", { name: "Country" }).getByRole("switch").nth(1).click(); | ||
|
|
||
| // Fill Contact Info Question | ||
| await page | ||
| .locator("div") | ||
| .filter({ hasText: new RegExp(`^${addQuestion}$`) }) | ||
| .nth(1) | ||
| .click(); | ||
| await page.getByRole("button", { name: "Contact Info" }).click(); | ||
| await page.getByLabel("Question*").fill(params.contactInfo.question); | ||
| await page.getByRole("row", { name: "Last Name" }).getByRole("switch").nth(1).click(); | ||
| await page.getByRole("row", { name: "Email" }).getByRole("switch").nth(1).click(); | ||
| await page.getByRole("row", { name: "Phone" }).getByRole("switch").nth(1).click(); | ||
| await page.getByRole("row", { name: "Company" }).getByRole("switch").nth(1).click(); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor repetitive code by iterating over field names
The code for toggling fields in both the Address and Contact Info questions is repetitive. To enhance maintainability and reduce duplication, consider refactoring the code to loop over arrays of field names.
Here's how you might refactor the code:
// Fill Address Question
await page
.locator("div")
.filter({ hasText: new RegExp(`^${addQuestion}$`) })
.nth(1)
.click();
await page.getByRole("button", { name: "Address" }).click();
await page.getByLabel("Question*").fill(params.address.question);
+const addressFields = ["Address Line 2", "City", "State", "Zip", "Country"];
+for (const field of addressFields) {
+ await page.getByRole("row", { name: field }).getByRole("switch").nth(1).click();
+}
// Fill Contact Info Question
await page
.locator("div")
.filter({ hasText: new RegExp(`^${addQuestion}$`) })
.nth(1)
.click();
await page.getByRole("button", { name: "Contact Info" }).click();
await page.getByLabel("Question*").fill(params.contactInfo.question);
+const contactFields = ["Last Name", "Email", "Phone", "Company"];
+for (const field of contactFields) {
+ await page.getByRole("row", { name: field }).getByRole("switch").nth(1).click();
+}This refactoring reduces code duplication and enhances readability, making it easier to manage and update the list of fields in the future.
📝 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.
| await page.getByRole("row", { name: "Address Line 2" }).getByRole("switch").nth(1).click(); | |
| await page.getByRole("row", { name: "City" }).getByRole("cell").nth(2).click(); | |
| await page.getByRole("row", { name: "State" }).getByRole("switch").nth(1).click(); | |
| await page.getByRole("row", { name: "Zip" }).getByRole("cell").nth(2).click(); | |
| await page.getByRole("row", { name: "Country" }).getByRole("switch").nth(1).click(); | |
| // Fill Contact Info Question | |
| await page | |
| .locator("div") | |
| .filter({ hasText: new RegExp(`^${addQuestion}$`) }) | |
| .nth(1) | |
| .click(); | |
| await page.getByRole("button", { name: "Contact Info" }).click(); | |
| await page.getByLabel("Question*").fill(params.contactInfo.question); | |
| await page.getByRole("row", { name: "Last Name" }).getByRole("switch").nth(1).click(); | |
| await page.getByRole("row", { name: "Email" }).getByRole("switch").nth(1).click(); | |
| await page.getByRole("row", { name: "Phone" }).getByRole("switch").nth(1).click(); | |
| await page.getByRole("row", { name: "Company" }).getByRole("switch").nth(1).click(); | |
| // Fill Address Question | |
| await page | |
| .locator("div") | |
| .filter({ hasText: new RegExp(`^${addQuestion}$`) }) | |
| .nth(1) | |
| .click(); | |
| await page.getByRole("button", { name: "Address" }).click(); | |
| await page.getByLabel("Question*").fill(params.address.question); | |
| const addressFields = ["Address Line 2", "City", "State", "Zip", "Country"]; | |
| for (const field of addressFields) { | |
| await page.getByRole("row", { name: field }).getByRole("switch").nth(1).click(); | |
| } | |
| // Fill Contact Info Question | |
| await page | |
| .locator("div") | |
| .filter({ hasText: new RegExp(`^${addQuestion}$`) }) | |
| .nth(1) | |
| .click(); | |
| await page.getByRole("button", { name: "Contact Info" }).click(); | |
| await page.getByLabel("Question*").fill(params.contactInfo.question); | |
| const contactFields = ["Last Name", "Email", "Phone", "Company"]; | |
| for (const field of contactFields) { | |
| await page.getByRole("row", { name: field }).getByRole("switch").nth(1).click(); | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (4)
packages/types/responses.ts (1)
137-139: LGTM! Consider adding a comment for clarity.The new
ZResponseFilterCriteriaFilledOutschema is well-defined and consistent with other filter criteria schemas. It aligns with the PR objective of adding a new contact info question type.For consistency with other schemas in this file, consider adding a brief comment explaining the purpose of this filter criteria. For example:
// Filter criteria for checking if a field is filled out const ZResponseFilterCriteriaFilledOut = z.object({ op: z.literal("filledOut"), });apps/web/app/(app)/environments/[environmentId]/surveys/[surveyId]/(analysis)/responses/components/ResponseTableColumns.tsx (2)
38-53: LGTM with a minor suggestion.The
getContactInfoFieldLabelfunction is well-structured and consistent with the existing codebase. However, consider handling thedefaultcase explicitly to prevent potential undefined behavior.Consider updating the
defaultcase to return a generic label or throw an error for unexpected fields:const getContactInfoFieldLabel = (field: string) => { switch (field) { case "firstName": return "First Name"; case "lastName": return "Last Name"; case "email": return "Email"; case "phone": return "Phone"; case "company": return "Company"; default: - break; + return "Unknown Field"; } };
108-130: LGTM with suggestions for improvement.The implementation of the "contactInfo" case is consistent with the existing code patterns and correctly handles the rendering of contact info fields. However, there are a few points to consider:
The
accessorKeyis using the field name directly. Ensure this matches the actual data structure of the response.Consider wrapping the
contactInfoFieldsdeclaration in a block to address the static analysis warning about switch clause declarations.Consider the following improvements:
- Ensure the
accessorKeymatches the actual response data structure:return { - accessorKey: contactInfoField, + accessorKey: `contactInfo.${contactInfoField}`, // ... rest of the code };
- Wrap the
contactInfoFieldsdeclaration in a block:case "contactInfo": { const contactInfoFields = ["firstName", "lastName", "email", "phone", "company"]; return contactInfoFields.map((contactInfoField) => { // ... rest of the code }); }
- Consider adding a type guard to ensure
responseValueis a string before rendering:cell: ({ row }) => { - const responseValue = row.original.responseData[contactInfoField]; + const responseValue = row.original.responseData.contactInfo?.[contactInfoField]; if (typeof responseValue === "string") { return <p className="text-slate-900">{responseValue}</p>; } + return null; },These changes will improve type safety and address the static analysis warning.
🧰 Tools
🪛 Biome
[error] 109-109: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.The declaration is defined in this switch clause:
Unsafe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
apps/web/playwright/utils/helper.ts (1)
295-306: Correct implementation, but refactoring opportunity exists.The Contact Info question implementation is correct and consistent with the Address question. However, there's an opportunity to reduce code duplication by refactoring both the Address and Contact Info question setup.
Consider implementing a helper function that takes an array of field names and toggles them, which can be used for both question types. This would improve maintainability and reduce the likelihood of errors in future updates.
Here's a suggested refactor:
const toggleFields = async (page: Page, fieldNames: string[]) => { for (const field of fieldNames) { await page.getByRole("row", { name: field }).getByRole("switch").nth(1).click(); } }; // Usage for Address const addressFields = ["Address Line 2", "City", "State", "Zip", "Country"]; await toggleFields(page, addressFields); // Usage for Contact Info const contactInfoFields = ["Last Name", "Email", "Phone", "Company"]; await toggleFields(page, contactInfoFields);This refactoring will make the code more concise and easier to maintain.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (11)
- apps/web/app/(app)/(survey-editor)/environments/[environmentId]/surveys/[surveyId]/edit/components/QuestionCard.tsx (3 hunks)
- apps/web/app/(app)/environments/[environmentId]/surveys/[surveyId]/(analysis)/responses/components/ResponseDataView.tsx (2 hunks)
- apps/web/app/(app)/environments/[environmentId]/surveys/[surveyId]/(analysis)/responses/components/ResponseTableColumns.tsx (2 hunks)
- apps/web/playwright/survey.spec.ts (1 hunks)
- apps/web/playwright/utils/helper.ts (1 hunks)
- apps/web/playwright/utils/mock.ts (1 hunks)
- packages/database/package.json (1 hunks)
- packages/lib/response/utils.ts (4 hunks)
- packages/lib/utils/questions.tsx (3 hunks)
- packages/types/responses.ts (2 hunks)
- packages/types/surveys/types.ts (7 hunks)
🚧 Files skipped from review as they are similar to previous changes (7)
- apps/web/app/(app)/(survey-editor)/environments/[environmentId]/surveys/[surveyId]/edit/components/QuestionCard.tsx
- apps/web/app/(app)/environments/[environmentId]/surveys/[surveyId]/(analysis)/responses/components/ResponseDataView.tsx
- apps/web/playwright/survey.spec.ts
- apps/web/playwright/utils/mock.ts
- packages/database/package.json
- packages/lib/utils/questions.tsx
- packages/types/surveys/types.ts
🧰 Additional context used
🪛 Biome
apps/web/app/(app)/environments/[environmentId]/surveys/[surveyId]/(analysis)/responses/components/ResponseTableColumns.tsx
[error] 109-109: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.The declaration is defined in this switch clause:
Unsafe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
🔇 Additional comments (6)
packages/types/responses.ts (2)
178-178: LGTM! Correct integration of the new filter criteria.The addition of
ZResponseFilterCriteriaFilledOutto thedatafield ofZResponseFilterCriteriais correct and consistent with the existing structure. This change properly integrates the new filter criteria for use in response filtering operations.
137-139: Summary: New filter criteria successfully addedThe changes in this file successfully introduce a new filter criteria for checking if a field is filled out. This addition is well-implemented and consistent with the existing code structure. It aligns with the PR objective of adding a new contact info question type.
Key points:
- New
ZResponseFilterCriteriaFilledOutschema added.ZResponseFilterCriteriaschema updated to include the new filter criteria.These changes enhance the filtering capabilities of the system, particularly for the new contact info question type. The implementation is clean and doesn't introduce any apparent issues or inconsistencies.
Also applies to: 178-178
apps/web/app/(app)/environments/[environmentId]/surveys/[surveyId]/(analysis)/responses/components/ResponseTableColumns.tsx (1)
Line range hint
1-430: Overall, the changes look good and align with the PR objectives.The implementation of the new "contactInfo" question type is well-integrated into the existing codebase. It follows established patterns and maintains consistency with other question types. The suggested improvements are minor and aim to enhance type safety and code structure.
A few points to consider for future development:
As the number of question types grows, consider refactoring the
getQuestionColumnsDatafunction to use a strategy pattern or separate handlers for each question type. This would improve maintainability and make it easier to add new question types in the future.Consider adding unit tests for the new
getContactInfoFieldLabelfunction and the "contactInfo" case ingetQuestionColumnsDatato ensure proper functionality and prevent regressions.Update the documentation to reflect the addition of the new contact info question type, including any specific behaviors or limitations users should be aware of.
🧰 Tools
🪛 Biome
[error] 109-109: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.The declaration is defined in this switch clause:
Unsafe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
apps/web/playwright/utils/helper.ts (2)
289-293: Implementation looks good, but consider refactoring.The implementation for toggling Address fields is correct and consistent. However, as mentioned in a previous review comment, consider refactoring this repetitive code by iterating over an array of field names to enhance maintainability.
289-306: Summary: New question types added successfully, refactoring recommended.The changes successfully introduce Address and Contact Info question types to the survey creation process. The implementations are correct and consistent. However, to improve code quality and maintainability, consider refactoring the repetitive code in both new question types as suggested in the previous comments.
Overall, these additions enhance the functionality of the survey creation process and align well with the PR objectives of adding a new Contact Info question type.
packages/lib/response/utils.ts (1)
Line range hint
1282-1300: Consider combining Address and ContactInfo cases to reduce duplication.The implementation for ContactInfo is correct and follows the existing pattern. However, as noted in a previous review comment, the code for Address and ContactInfo cases is identical. To improve maintainability and reduce code duplication, consider combining these cases.
Here's a suggested refactoring:
- case TSurveyQuestionTypeEnum.Address: - case TSurveyQuestionTypeEnum.ContactInfo: { + case TSurveyQuestionTypeEnum.Address: + case TSurveyQuestionTypeEnum.ContactInfo: { let values: TSurveyQuestionSummaryAddress["samples"] = []; responses.forEach((response) => { const answer = response.data[question.id]; if (Array.isArray(answer) && answer.length > 0) { values.push({ id: response.id, updatedAt: response.updatedAt, value: answer, person: response.person, personAttributes: response.personAttributes, }); } }); summary.push({ - type: question.type as TSurveyQuestionTypeEnum.ContactInfo, - question: question as TSurveyContactInfoQuestion, + type: question.type, + question, responseCount: values.length, samples: values.slice(0, VALUES_LIMIT), }); values = []; break; }This change will handle both Address and ContactInfo cases with the same code, reducing duplication and improving maintainability.
…mbricks into feat/contact-info
2 participants
What does this PR do?
Fixes https://github.com/formbricks/internal/issues/168
How should this be tested?
Checklist
Required
pnpm buildconsole.logsgit pull origin mainAppreciated
Summary by CodeRabbit
Release Notes
New Features
ContactInfoQuestionFormcomponent for editing contact information questions in surveys.ContactInfoquestion type, requiring fields for first name, last name, email, phone, and company.AddressQuestionForm.QuestionToggleTablecomponent to manage visibility and requirement status of fields.Improvements
ContactInfo.Bug Fixes
Style
Documentation