Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎ 2 Skipped Deployments
|
|
Thank you for following the naming conventions for pull request titles! 🙏 |
…feat-advanced-logic-editor
…iables on each response
|
hey Piyush, I'll pre-review this to save us a feedback loop 🤓 It's looking great already! Here's what I noticed?
2.b. Icon size and color: These icons can be a bit smaller and darker for better usability
also gap here is too wide and inconsistent with gap between Condition and Action I think you can get rid of
here as well:
and there is a double border separating the input from the dropdown
8.b. This icon needs more space on the right:
10 Update Delete icon to match rest of survey editor: vs 
That's it in terms of UI / UX. I'll do a proper test when you've marked the PR ready for review 🤓 Thanks, great work!! |
…feat-advanced-logic-editor
…feat-advanced-logic-editor
…feat-advanced-logic-editor
…feat-advanced-logic-editor
![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: 6
🧹 Outside diff range and nitpick comments (2)
packages/ui/components/InputCombobox/stories.ts (1)
37-49: LGTM: Default story covers key features.The Default story effectively demonstrates the InputCombobox's main features, including search, icons, and clearable selection.
Consider adding a comment above the
onChangeValueprop to indicate that the console.log is for demonstration purposes only and should not be used in production code.packages/ui/components/InputCombobox/index.tsx (1)
48-63: Avoid defaulting theidprop to a static valueThe
idprop is marked as required inInputComboboxProps, but a default value of"temp"is provided (line 49). This could lead to multiple instances of the component sharing the sameid, which may cause accessibility issues. It's better to enforce providing a uniqueidfor each instance.Consider removing the default value for
id:export const InputCombobox = ({ - id = "temp", + id, showSearch = true, searchPlaceholder = "Search...",
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
- apps/web/app/(app)/(survey-editor)/environments/[environmentId]/surveys/[surveyId]/edit/components/LogicEditorActions.tsx (1 hunks)
- apps/web/app/(app)/(survey-editor)/environments/[environmentId]/surveys/[surveyId]/edit/components/LogicEditorConditions.tsx (1 hunks)
- apps/web/app/(app)/(survey-editor)/environments/[environmentId]/surveys/[surveyId]/edit/lib/utils.tsx (1 hunks)
- packages/ui/components/InputCombobox/index.tsx (1 hunks)
- packages/ui/components/InputCombobox/stories.ts (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- apps/web/app/(app)/(survey-editor)/environments/[environmentId]/surveys/[surveyId]/edit/components/LogicEditorActions.tsx
- apps/web/app/(app)/(survey-editor)/environments/[environmentId]/surveys/[surveyId]/edit/components/LogicEditorConditions.tsx
🧰 Additional context used
🪛 Biome
apps/web/app/(app)/(survey-editor)/environments/[environmentId]/surveys/[surveyId]/edit/lib/utils.tsx
[error] 45-45: Avoid the use of spread (
...) syntax on accumulators.Spread syntax should be avoided on accumulators (like those in
.reduce) because it causes a time complexity ofO(n^2).
Consider methods such as .splice or .push instead.(lint/performance/noAccumulatingSpread)
packages/ui/components/InputCombobox/index.tsx
[error] 176-176: 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)
[error] 297-297: 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)
🔇 Additional comments (7)
packages/ui/components/InputCombobox/stories.ts (6)
1-29: LGTM: Imports and metadata are well-structured.The imports and metadata for the InputCombobox stories are appropriately defined. The metadata provides a clear and comprehensive description of the component's features, which is valuable for documentation.
31-35: LGTM: Common options are well-defined.The
commonOptionsarray is well-structured and promotes code reusability across multiple stories. Each option includes a label, value, and icon, which is consistent and informative.
51-88: LGTM: WithInput, GroupedOptions, and MultiSelect stories are well-implemented.These stories effectively demonstrate different configurations of the InputCombobox:
- WithInput shows the component with an input field.
- GroupedOptions showcases the ability to group options, which is a valuable feature.
- MultiSelect demonstrates the multi-selection capability.
The use of
...Default.argspromotes code reusability and maintains consistency across stories.
90-96: LGTM: Clearable story demonstrates the clearable feature.The Clearable story effectively shows the component's ability to clear a pre-selected value. It builds upon the Default story, modifying only the necessary props to showcase this feature.
98-103: LGTM: WithoutSearch story demonstrates the component without search.The WithoutSearch story effectively shows the InputCombobox without search functionality. It appropriately reuses the Default args and modifies only the
showSearchprop to showcase this configuration.
1-103: Overall: Excellent implementation of Storybook stories for InputCombobox.This file provides a comprehensive set of stories that effectively demonstrate the various features and configurations of the InputCombobox component. The code is well-structured, consistent, and promotes reusability. It aligns well with Storybook best practices and will serve as valuable documentation for the component.
Some key strengths:
- Clear and informative metadata
- Reusable common options
- Stories covering different configurations (default, with input, grouped options, multi-select, clearable, without search)
- Consistent use of props across stories
Great job on creating these stories!
apps/web/app/(app)/(survey-editor)/environments/[environmentId]/surveys/[surveyId]/edit/lib/utils.tsx (1)
26-26:⚠️ Potential issueCorrect the regex pattern in
formatTextWithSlashesThe regular expression in
formatTextWithSlashesmay not correctly match text enclosed in slashes due to improper escaping. The ending slash should not be escaped. The corrected regex ensures that text between slashes is properly captured.Apply this diff to fix the regex pattern:
- const regex = /\/(.*?)\\/g; + const regex = /\/(.*?)\//g;Likely invalid or redundant comment.
| const isSelected = (option: TComboboxOption) => { | ||
| if (typeof localValue === "object") { | ||
| if (Array.isArray(localValue)) { | ||
| return localValue.find((item) => item === option.value) !== undefined; | ||
| } | ||
|
|
||
| return localValue === option.value; | ||
| } | ||
| }; |
There was a problem hiding this comment.
Ensure isSelected function always returns a boolean
The isSelected function may not return a value in all code paths. If localValue is not an object, the function does not return anything, which results in undefined. To avoid potential bugs, the function should always return a boolean value.
Apply this diff to return false when localValue is not an object:
const isSelected = (option: TComboboxOption) => {
if (typeof localValue === "object") {
if (Array.isArray(localValue)) {
return localValue.find((item) => item === option.value) !== undefined;
}
return localValue === option.value;
}
+ return false;
};📝 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.
| const isSelected = (option: TComboboxOption) => { | |
| if (typeof localValue === "object") { | |
| if (Array.isArray(localValue)) { | |
| return localValue.find((item) => item === option.value) !== undefined; | |
| } | |
| return localValue === option.value; | |
| } | |
| }; | |
| const isSelected = (option: TComboboxOption) => { | |
| if (typeof localValue === "object") { | |
| if (Array.isArray(localValue)) { | |
| return localValue.find((item) => item === option.value) !== undefined; | |
| } | |
| return localValue === option.value; | |
| } | |
| return false; | |
| }; |
| return localValue.map((item, idx) => { | ||
| const option = validOptions?.find((opt) => opt.value === item); | ||
|
|
||
| if (!option) { | ||
| return null; | ||
| } | ||
|
|
||
| return ( | ||
| <> | ||
| {idx !== 0 && <span>,</span>} | ||
| <div className="flex items-center gap-2"> | ||
| {option.icon && <option.icon className="h-5 w-5 shrink-0 text-slate-400" />} | ||
| {option.imgSrc && <Image src={option.imgSrc} alt={option.label} width={24} height={24} />} | ||
| <span>{option.label}</span> | ||
| </div> | ||
| </> | ||
| ); | ||
| }); | ||
| } else { |
There was a problem hiding this comment.
Add missing key prop to elements in list rendering
In the localValue.map function (lines 166-184), the returned elements are wrapped in fragments <>...</>, which are missing a key prop. React requires that elements in a list have a unique key prop to help identify which items have changed.
Apply this diff to fix the missing key prop:
return localValue.map((item, idx) => {
const option = validOptions?.find((opt) => opt.value === item);
if (!option) {
return null;
}
- return (
- <>
+ return (
+ <React.Fragment key={item}>
{idx !== 0 && <span>,</span>}
<div className="flex items-center gap-2">
{option.icon && <option.icon className="h-5 w-5 shrink-0 text-slate-400" />}
{option.imgSrc && <Image src={option.imgSrc} alt={option.label} width={24} height={24} />}
<span>{option.label}</span>
</div>
- </>
+ </React.Fragment>
);
});📝 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.
| return localValue.map((item, idx) => { | |
| const option = validOptions?.find((opt) => opt.value === item); | |
| if (!option) { | |
| return null; | |
| } | |
| return ( | |
| <> | |
| {idx !== 0 && <span>,</span>} | |
| <div className="flex items-center gap-2"> | |
| {option.icon && <option.icon className="h-5 w-5 shrink-0 text-slate-400" />} | |
| {option.imgSrc && <Image src={option.imgSrc} alt={option.label} width={24} height={24} />} | |
| <span>{option.label}</span> | |
| </div> | |
| </> | |
| ); | |
| }); | |
| } else { | |
| return localValue.map((item, idx) => { | |
| const option = validOptions?.find((opt) => opt.value === item); | |
| if (!option) { | |
| return null; | |
| } | |
| return ( | |
| <React.Fragment key={item}> | |
| {idx !== 0 && <span>,</span>} | |
| <div className="flex items-center gap-2"> | |
| {option.icon && <option.icon className="h-5 w-5 shrink-0 text-slate-400" />} | |
| {option.imgSrc && <Image src={option.imgSrc} alt={option.label} width={24} height={24} />} | |
| <span>{option.label}</span> | |
| </div> | |
| </React.Fragment> | |
| ); | |
| }); | |
| } else { |
🧰 Tools
🪛 Biome
[error] 176-176: 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)
| <> | ||
| {idx !== 0 && <CommandSeparator key={idx} className="bg-slate-300" />} | ||
| <CommandGroup heading={group.label}> | ||
| {group.options.map((option) => ( | ||
| <CommandItem | ||
| key={option.value} | ||
| onSelect={() => handleSelect(option)} | ||
| className="cursor-pointer truncate"> | ||
| {showCheckIcon && isSelected(option) && ( | ||
| <CheckIcon className="mr-2 h-4 w-4 text-slate-300 hover:text-slate-400" /> | ||
| )} | ||
| {option.icon && <option.icon className="mr-2 h-5 w-5 shrink-0 text-slate-400" />} | ||
| {option.imgSrc && ( | ||
| <Image | ||
| src={option.imgSrc} | ||
| alt={option.label} | ||
| width={24} | ||
| height={24} | ||
| className="mr-2 shrink-0" | ||
| /> | ||
| )} | ||
| <span className="truncate">{option.label}</span> | ||
| </CommandItem> | ||
| ))} | ||
| </CommandGroup> | ||
| </> | ||
| ))} |
There was a problem hiding this comment.
Add missing key prop to elements in list rendering
In the groupedOptions.map function (lines 295-321), the returned elements are wrapped in fragments <>...</>, which are missing a key prop. Each element in a list should have a unique key prop to help React track changes.
Apply this diff to fix the missing key prop:
{groupedOptions?.map((group, idx) => (
- <>
+ <React.Fragment key={group.value}>
{idx !== 0 && <CommandSeparator key={idx} className="bg-slate-300" />}
<CommandGroup heading={group.label}>
{group.options.map((option) => (
<CommandItem
key={option.value}
onSelect={() => handleSelect(option)}
className="cursor-pointer truncate">
{showCheckIcon && isSelected(option) && (
<CheckIcon className="mr-2 h-4 w-4 text-slate-300 hover:text-slate-400" />
)}
{option.icon && <option.icon className="mr-2 h-5 w-5 shrink-0 text-slate-400" />}
{option.imgSrc && (
<Image
src={option.imgSrc}
alt={option.label}
width={24}
height={24}
className="mr-2 shrink-0"
/>
)}
<span className="truncate">{option.label}</span>
</CommandItem>
))}
</CommandGroup>
- </>
+ </React.Fragment>
))}📝 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.
| <> | |
| {idx !== 0 && <CommandSeparator key={idx} className="bg-slate-300" />} | |
| <CommandGroup heading={group.label}> | |
| {group.options.map((option) => ( | |
| <CommandItem | |
| key={option.value} | |
| onSelect={() => handleSelect(option)} | |
| className="cursor-pointer truncate"> | |
| {showCheckIcon && isSelected(option) && ( | |
| <CheckIcon className="mr-2 h-4 w-4 text-slate-300 hover:text-slate-400" /> | |
| )} | |
| {option.icon && <option.icon className="mr-2 h-5 w-5 shrink-0 text-slate-400" />} | |
| {option.imgSrc && ( | |
| <Image | |
| src={option.imgSrc} | |
| alt={option.label} | |
| width={24} | |
| height={24} | |
| className="mr-2 shrink-0" | |
| /> | |
| )} | |
| <span className="truncate">{option.label}</span> | |
| </CommandItem> | |
| ))} | |
| </CommandGroup> | |
| </> | |
| ))} | |
| {groupedOptions?.map((group, idx) => ( | |
| <React.Fragment key={group.value}> | |
| {idx !== 0 && <CommandSeparator key={idx} className="bg-slate-300" />} | |
| <CommandGroup heading={group.label}> | |
| {group.options.map((option) => ( | |
| <CommandItem | |
| key={option.value} | |
| onSelect={() => handleSelect(option)} | |
| className="cursor-pointer truncate"> | |
| {showCheckIcon && isSelected(option) && ( | |
| <CheckIcon className="mr-2 h-4 w-4 text-slate-300 hover:text-slate-400" /> | |
| )} | |
| {option.icon && <option.icon className="mr-2 h-5 w-5 shrink-0 text-slate-400" />} | |
| {option.imgSrc && ( | |
| <Image | |
| src={option.imgSrc} | |
| alt={option.label} | |
| width={24} | |
| height={24} | |
| className="mr-2 shrink-0" | |
| /> | |
| )} | |
| <span className="truncate">{option.label}</span> | |
| </CommandItem> | |
| ))} | |
| </CommandGroup> | |
| </React.Fragment> | |
| ))} |
🧰 Tools
🪛 Biome
[error] 297-297: 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)
| export const getMatchValueProps = ( | ||
| condition: TSingleCondition, | ||
| localSurvey: TSurvey | ||
| ): { | ||
| show?: boolean; | ||
| showInput?: boolean; | ||
| inputType?: HTMLInputTypeAttribute; | ||
| options: TComboboxGroupedOption[]; | ||
| } => { | ||
| if ( | ||
| [ | ||
| "isAccepted", | ||
| "isBooked", | ||
| "isClicked", | ||
| "isCompletelySubmitted", | ||
| "isPartiallySubmitted", | ||
| "isSkipped", | ||
| "isSubmitted", | ||
| ].includes(condition.operator) | ||
| ) { | ||
| return { show: false, options: [] }; | ||
| } | ||
|
|
||
| let questions = localSurvey.questions ?? []; | ||
| let variables = localSurvey.variables ?? []; | ||
| let hiddenFields = localSurvey.hiddenFields?.fieldIds ?? []; | ||
|
|
||
| const selectedQuestion = questions.find((question) => question.id === condition.leftOperand.value); | ||
| const selectedVariable = variables.find((variable) => variable.id === condition.leftOperand.value); | ||
|
|
||
| if (condition.leftOperand.type === "question") { | ||
| questions = questions.filter((question) => question.id !== condition.leftOperand.value); | ||
| } else if (condition.leftOperand.type === "variable") { | ||
| variables = variables.filter((variable) => variable.id !== condition.leftOperand.value); | ||
| } else if (condition.leftOperand.type === "hiddenField") { | ||
| hiddenFields = hiddenFields.filter((field) => field !== condition.leftOperand.value); | ||
| } | ||
|
|
||
| if (condition.leftOperand.type === "question") { | ||
| if (selectedQuestion?.type === TSurveyQuestionTypeEnum.OpenText) { | ||
| const allowedQuestionTypes = [TSurveyQuestionTypeEnum.OpenText]; | ||
|
|
||
| if (selectedQuestion.inputType === "number") { | ||
| allowedQuestionTypes.push(TSurveyQuestionTypeEnum.Rating, TSurveyQuestionTypeEnum.NPS); | ||
| } | ||
|
|
||
| if (["equals", "doesNotEqual"].includes(condition.operator)) { | ||
| if (selectedQuestion.inputType !== "number") { | ||
| allowedQuestionTypes.push( | ||
| TSurveyQuestionTypeEnum.Date, | ||
| TSurveyQuestionTypeEnum.MultipleChoiceSingle, | ||
| TSurveyQuestionTypeEnum.MultipleChoiceMulti | ||
| ); | ||
| } | ||
| } | ||
|
|
||
| const allowedQuestions = questions.filter((question) => allowedQuestionTypes.includes(question.type)); | ||
|
|
||
| const questionOptions = allowedQuestions.map((question) => { | ||
| return { | ||
| icon: questionIconMapping[question.type], | ||
| label: getLocalizedValue(question.headline, "default"), | ||
| value: question.id, | ||
| meta: { | ||
| type: "question", | ||
| }, | ||
| }; | ||
| }); | ||
|
|
||
| const variableOptions = variables | ||
| .filter((variable) => | ||
| selectedQuestion.inputType !== "number" ? variable.type === "text" : variable.type === "number" | ||
| ) | ||
| .map((variable) => { | ||
| return { | ||
| icon: variable.type === "number" ? FileDigitIcon : FileType2Icon, | ||
| label: variable.name, | ||
| value: variable.id, | ||
| meta: { | ||
| type: "variable", | ||
| }, | ||
| }; | ||
| }); | ||
|
|
||
| const hiddenFieldsOptions = hiddenFields.map((field) => { | ||
| return { | ||
| icon: EyeOffIcon, | ||
| label: field, | ||
| value: field, | ||
| meta: { | ||
| type: "hiddenField", | ||
| }, | ||
| }; | ||
| }); | ||
|
|
||
| const groupedOptions: TComboboxGroupedOption[] = []; | ||
|
|
||
| if (questionOptions.length > 0) { | ||
| groupedOptions.push({ | ||
| label: "Questions", | ||
| value: "questions", | ||
| options: questionOptions, | ||
| }); | ||
| } | ||
|
|
||
| if (variableOptions.length > 0) { | ||
| groupedOptions.push({ | ||
| label: "Variables", | ||
| value: "variables", | ||
| options: variableOptions, | ||
| }); | ||
| } | ||
|
|
||
| if (hiddenFieldsOptions.length > 0) { | ||
| groupedOptions.push({ | ||
| label: "Hidden Fields", | ||
| value: "hiddenFields", | ||
| options: hiddenFieldsOptions, | ||
| }); | ||
| } | ||
| return { | ||
| show: true, | ||
| showInput: true, | ||
| inputType: selectedQuestion.inputType === "number" ? "number" : "text", | ||
| options: groupedOptions, | ||
| }; | ||
| } else if ( | ||
| selectedQuestion?.type === TSurveyQuestionTypeEnum.MultipleChoiceSingle || | ||
| selectedQuestion?.type === TSurveyQuestionTypeEnum.MultipleChoiceMulti | ||
| ) { | ||
| const choices = selectedQuestion.choices.map((choice) => { | ||
| return { | ||
| label: getLocalizedValue(choice.label, "default"), | ||
| value: choice.id, | ||
| meta: { | ||
| type: "static", | ||
| }, | ||
| }; | ||
| }); | ||
|
|
||
| return { | ||
| show: true, | ||
| showInput: false, | ||
| options: [{ label: "Choices", value: "choices", options: choices }], | ||
| }; | ||
| } else if (selectedQuestion?.type === TSurveyQuestionTypeEnum.PictureSelection) { | ||
| const choices = selectedQuestion.choices.map((choice, idx) => { | ||
| return { | ||
| imgSrc: choice.imageUrl, | ||
| label: `Picture ${idx + 1}`, | ||
| value: choice.id, | ||
| meta: { | ||
| type: "static", | ||
| }, | ||
| }; | ||
| }); | ||
|
|
||
| return { | ||
| show: true, | ||
| showInput: false, | ||
| options: [{ label: "Choices", value: "choices", options: choices }], | ||
| }; | ||
| } else if (selectedQuestion?.type === TSurveyQuestionTypeEnum.Rating) { | ||
| const choices = Array.from({ length: selectedQuestion.range }, (_, idx) => { | ||
| return { | ||
| label: `${idx + 1}`, | ||
| value: idx + 1, | ||
| meta: { | ||
| type: "static", | ||
| }, | ||
| }; | ||
| }); | ||
|
|
||
| const numberVariables = variables.filter((variable) => variable.type === "number"); | ||
|
|
||
| const variableOptions = numberVariables.map((variable) => { | ||
| return { | ||
| icon: FileDigitIcon, | ||
| label: variable.name, | ||
| value: variable.id, | ||
| meta: { | ||
| type: "variable", | ||
| }, | ||
| }; | ||
| }); | ||
|
|
||
| const groupedOptions: TComboboxGroupedOption[] = []; | ||
|
|
||
| if (choices.length > 0) { | ||
| groupedOptions.push({ | ||
| label: "Choices", | ||
| value: "choices", | ||
| options: choices, | ||
| }); | ||
| } | ||
|
|
||
| if (variableOptions.length > 0) { | ||
| groupedOptions.push({ | ||
| label: "Variables", | ||
| value: "variables", | ||
| options: variableOptions, | ||
| }); | ||
| } | ||
|
|
||
| return { | ||
| show: true, | ||
| showInput: false, | ||
| options: groupedOptions, | ||
| }; | ||
| } else if (selectedQuestion?.type === TSurveyQuestionTypeEnum.NPS) { | ||
| const choices = Array.from({ length: 11 }, (_, idx) => { | ||
| return { | ||
| label: `${idx}`, | ||
| value: idx, | ||
| meta: { | ||
| type: "static", | ||
| }, | ||
| }; | ||
| }); | ||
|
|
||
| const numberVariables = variables.filter((variable) => variable.type === "number"); | ||
|
|
||
| const variableOptions = numberVariables.map((variable) => { | ||
| return { | ||
| icon: FileDigitIcon, | ||
| label: variable.name, | ||
| value: variable.id, | ||
| meta: { | ||
| type: "variable", | ||
| }, | ||
| }; | ||
| }); | ||
|
|
||
| const groupedOptions: TComboboxGroupedOption[] = []; | ||
|
|
||
| if (choices.length > 0) { | ||
| groupedOptions.push({ | ||
| label: "Choices", | ||
| value: "choices", | ||
| options: choices, | ||
| }); | ||
| } | ||
|
|
||
| if (variableOptions.length > 0) { | ||
| groupedOptions.push({ | ||
| label: "Variables", | ||
| value: "variables", | ||
| options: variableOptions, | ||
| }); | ||
| } | ||
|
|
||
| return { | ||
| show: true, | ||
| showInput: false, | ||
| options: groupedOptions, | ||
| }; | ||
| } else if (selectedQuestion?.type === TSurveyQuestionTypeEnum.Date) { | ||
| const openTextQuestions = questions.filter((question) => | ||
| [TSurveyQuestionTypeEnum.OpenText, TSurveyQuestionTypeEnum.Date].includes(question.type) | ||
| ); | ||
|
|
||
| const questionOptions = openTextQuestions.map((question) => { | ||
| return { | ||
| icon: questionIconMapping[question.type], | ||
| label: getLocalizedValue(question.headline, "default"), | ||
| value: question.id, | ||
| meta: { | ||
| type: "question", | ||
| }, | ||
| }; | ||
| }); | ||
|
|
||
| const stringVariables = variables.filter((variable) => variable.type === "text"); | ||
|
|
||
| const variableOptions = stringVariables.map((variable) => { | ||
| return { | ||
| icon: FileType2Icon, | ||
| label: variable.name, | ||
| value: variable.id, | ||
| meta: { | ||
| type: "variable", | ||
| }, | ||
| }; | ||
| }); | ||
|
|
||
| const hiddenFieldsOptions = hiddenFields.map((field) => { | ||
| return { | ||
| icon: EyeOffIcon, | ||
| label: field, | ||
| value: field, | ||
| meta: { | ||
| type: "hiddenField", | ||
| }, | ||
| }; | ||
| }); | ||
|
|
||
| const groupedOptions: TComboboxGroupedOption[] = []; | ||
|
|
||
| if (questionOptions.length > 0) { | ||
| groupedOptions.push({ | ||
| label: "Questions", | ||
| value: "questions", | ||
| options: questionOptions, | ||
| }); | ||
| } | ||
|
|
||
| if (variableOptions.length > 0) { | ||
| groupedOptions.push({ | ||
| label: "Variables", | ||
| value: "variables", | ||
| options: variableOptions, | ||
| }); | ||
| } | ||
|
|
||
| if (hiddenFieldsOptions.length > 0) { | ||
| groupedOptions.push({ | ||
| label: "Hidden Fields", | ||
| value: "hiddenFields", | ||
| options: hiddenFieldsOptions, | ||
| }); | ||
| } | ||
|
|
||
| return { | ||
| show: true, | ||
| showInput: true, | ||
| inputType: "date", | ||
| options: groupedOptions, | ||
| }; | ||
| } | ||
| } else if (condition.leftOperand.type === "variable") { | ||
| if (selectedVariable?.type === "text") { | ||
| const allowedQuestionTypes = [ | ||
| TSurveyQuestionTypeEnum.OpenText, | ||
| TSurveyQuestionTypeEnum.MultipleChoiceSingle, | ||
| ]; | ||
|
|
||
| if (["equals", "doesNotEqual"].includes(condition.operator)) { | ||
| allowedQuestionTypes.push(TSurveyQuestionTypeEnum.MultipleChoiceMulti, TSurveyQuestionTypeEnum.Date); | ||
| } | ||
|
|
||
| const allowedQuestions = questions.filter((question) => allowedQuestionTypes.includes(question.type)); | ||
|
|
||
| const questionOptions = allowedQuestions.map((question) => { | ||
| return { | ||
| icon: questionIconMapping[question.type], | ||
| label: getLocalizedValue(question.headline, "default"), | ||
| value: question.id, | ||
| meta: { | ||
| type: "question", | ||
| }, | ||
| }; | ||
| }); | ||
|
|
||
| const stringVariables = variables.filter((variable) => variable.type === "text"); | ||
|
|
||
| const variableOptions = stringVariables.map((variable) => { | ||
| return { | ||
| icon: FileType2Icon, | ||
| label: variable.name, | ||
| value: variable.id, | ||
| meta: { | ||
| type: "variable", | ||
| }, | ||
| }; | ||
| }); | ||
|
|
||
| const hiddenFieldsOptions = hiddenFields.map((field) => { | ||
| return { | ||
| icon: EyeOffIcon, | ||
| label: field, | ||
| value: field, | ||
| meta: { | ||
| type: "hiddenField", | ||
| }, | ||
| }; | ||
| }); | ||
|
|
||
| const groupedOptions: TComboboxGroupedOption[] = []; | ||
|
|
||
| if (questionOptions.length > 0) { | ||
| groupedOptions.push({ | ||
| label: "Questions", | ||
| value: "questions", | ||
| options: questionOptions, | ||
| }); | ||
| } | ||
|
|
||
| if (variableOptions.length > 0) { | ||
| groupedOptions.push({ | ||
| label: "Variables", | ||
| value: "variables", | ||
| options: variableOptions, | ||
| }); | ||
| } | ||
|
|
||
| if (hiddenFieldsOptions.length > 0) { | ||
| groupedOptions.push({ | ||
| label: "Hidden Fields", | ||
| value: "hiddenFields", | ||
| options: hiddenFieldsOptions, | ||
| }); | ||
| } | ||
|
|
||
| return { | ||
| show: true, | ||
| showInput: true, | ||
| inputType: "text", | ||
| options: groupedOptions, | ||
| }; | ||
| } else if (selectedVariable?.type === "number") { | ||
| const allowedQuestions = questions.filter((question) => | ||
| [TSurveyQuestionTypeEnum.Rating, TSurveyQuestionTypeEnum.NPS].includes(question.type) | ||
| ); | ||
|
|
||
| const questionOptions = allowedQuestions.map((question) => { | ||
| return { | ||
| icon: questionIconMapping[question.type], | ||
| label: getLocalizedValue(question.headline, "default"), | ||
| value: question.id, | ||
| meta: { | ||
| type: "question", | ||
| }, | ||
| }; | ||
| }); | ||
|
|
||
| const numberVariables = variables.filter((variable) => variable.type === "number"); | ||
|
|
||
| const variableOptions = numberVariables.map((variable) => { | ||
| return { | ||
| icon: FileDigitIcon, | ||
| label: variable.name, | ||
| value: variable.id, | ||
| meta: { | ||
| type: "variable", | ||
| }, | ||
| }; | ||
| }); | ||
|
|
||
| const hiddenFieldsOptions = hiddenFields.map((field) => { | ||
| return { | ||
| icon: EyeOffIcon, | ||
| label: field, | ||
| value: field, | ||
| meta: { | ||
| type: "hiddenField", | ||
| }, | ||
| }; | ||
| }); | ||
|
|
||
| const groupedOptions: TComboboxGroupedOption[] = []; | ||
|
|
||
| if (questionOptions.length > 0) { | ||
| groupedOptions.push({ | ||
| label: "Questions", | ||
| value: "questions", | ||
| options: questionOptions, | ||
| }); | ||
| } | ||
|
|
||
| if (variableOptions.length > 0) { | ||
| groupedOptions.push({ | ||
| label: "Variables", | ||
| value: "variables", | ||
| options: variableOptions, | ||
| }); | ||
| } | ||
|
|
||
| if (hiddenFieldsOptions.length > 0) { | ||
| groupedOptions.push({ | ||
| label: "Hidden Fields", | ||
| value: "hiddenFields", | ||
| options: hiddenFieldsOptions, | ||
| }); | ||
| } | ||
|
|
||
| return { | ||
| show: true, | ||
| showInput: true, | ||
| inputType: "number", | ||
| options: groupedOptions, | ||
| }; | ||
| } | ||
| } else if (condition.leftOperand.type === "hiddenField") { | ||
| const allowedQuestionTypes = [ | ||
| TSurveyQuestionTypeEnum.OpenText, | ||
| TSurveyQuestionTypeEnum.MultipleChoiceSingle, | ||
| ]; | ||
|
|
||
| if (["equals", "doesNotEqual"].includes(condition.operator)) { | ||
| allowedQuestionTypes.push(TSurveyQuestionTypeEnum.MultipleChoiceMulti, TSurveyQuestionTypeEnum.Date); | ||
| } | ||
|
|
||
| const allowedQuestions = questions.filter((question) => allowedQuestionTypes.includes(question.type)); | ||
|
|
||
| const questionOptions = allowedQuestions.map((question) => { | ||
| return { | ||
| icon: questionIconMapping[question.type], | ||
| label: getLocalizedValue(question.headline, "default"), | ||
| value: question.id, | ||
| meta: { | ||
| type: "question", | ||
| }, | ||
| }; | ||
| }); | ||
|
|
||
| const variableOptions = variables | ||
| .filter((variable) => variable.type === "text") | ||
| .map((variable) => { | ||
| return { | ||
| icon: FileType2Icon, | ||
| label: variable.name, | ||
| value: variable.id, | ||
| meta: { | ||
| type: "variable", | ||
| }, | ||
| }; | ||
| }); | ||
|
|
||
| const hiddenFieldsOptions = hiddenFields.map((field) => { | ||
| return { | ||
| icon: EyeOffIcon, | ||
| label: field, | ||
| value: field, | ||
| meta: { | ||
| type: "hiddenField", | ||
| }, | ||
| }; | ||
| }); | ||
|
|
||
| const groupedOptions: TComboboxGroupedOption[] = []; | ||
|
|
||
| if (questionOptions.length > 0) { | ||
| groupedOptions.push({ | ||
| label: "Questions", | ||
| value: "questions", | ||
| options: questionOptions, | ||
| }); | ||
| } | ||
|
|
||
| if (variableOptions.length > 0) { | ||
| groupedOptions.push({ | ||
| label: "Variables", | ||
| value: "variables", | ||
| options: variableOptions, | ||
| }); | ||
| } | ||
|
|
||
| if (hiddenFieldsOptions.length > 0) { | ||
| groupedOptions.push({ | ||
| label: "Hidden Fields", | ||
| value: "hiddenFields", | ||
| options: hiddenFieldsOptions, | ||
| }); | ||
| } | ||
|
|
||
| return { | ||
| show: true, | ||
| showInput: true, | ||
| inputType: "text", | ||
| options: groupedOptions, | ||
| }; | ||
| } | ||
|
|
||
| return { show: false, options: [] }; | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor the getMatchValueProps function to improve maintainability
The getMatchValueProps function spans over 560 lines and contains multiple nested conditional statements with repeated code blocks for handling different operand types and question types. This complexity makes the code difficult to read, maintain, and test.
Consider refactoring the function by extracting helper functions for repetitive code segments, such as generating grouped options or handling specific question types. Breaking down the function into smaller, focused functions will enhance readability and make future modifications easier.
For example, you could create a helper function to generate grouped options:
const generateGroupedOptions = (
questions: TSurveyQuestion[],
variables: TSurveyVariable[],
hiddenFields: string[]
): TComboboxGroupedOption[] => {
// Implementation to generate grouped options
};Then, use this helper function in getMatchValueProps wherever grouped options are constructed.
| export const getDefaultOperatorForQuestion = (question: TSurveyQuestion): TSurveyLogicConditionsOperator => { | ||
| const options = getQuestionOperatorOptions(question); | ||
|
|
||
| return options[0].value.toString() as TSurveyLogicConditionsOperator; | ||
| }; |
There was a problem hiding this comment.
Add a safety check for empty options array in getDefaultOperatorForQuestion
The function getDefaultOperatorForQuestion assumes that getQuestionOperatorOptions will always return a non-empty array. If options is empty, accessing options[0] will result in an undefined value, potentially causing runtime errors.
Consider adding a check to ensure that the options array is not empty before accessing options[0]. For example:
export const getDefaultOperatorForQuestion = (question: TSurveyQuestion): TSurveyLogicConditionsOperator => {
const options = getQuestionOperatorOptions(question);
+ if (options.length === 0) {
+ throw new Error(`No operator options available for question type ${question.type}`);
+ }
return options[0].value.toString() as TSurveyLogicConditionsOperator;
};📝 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 getDefaultOperatorForQuestion = (question: TSurveyQuestion): TSurveyLogicConditionsOperator => { | |
| const options = getQuestionOperatorOptions(question); | |
| return options[0].value.toString() as TSurveyLogicConditionsOperator; | |
| }; | |
| export const getDefaultOperatorForQuestion = (question: TSurveyQuestion): TSurveyLogicConditionsOperator => { | |
| const options = getQuestionOperatorOptions(question); | |
| if (options.length === 0) { | |
| throw new Error(`No operator options available for question type ${question.type}`); | |
| } | |
| return options[0].value.toString() as TSurveyLogicConditionsOperator; | |
| }; |
| export const getActionValueOptions = (variableId: string, localSurvey: TSurvey): TComboboxGroupedOption[] => { | ||
| const hiddenFields = localSurvey.hiddenFields?.fieldIds ?? []; | ||
| let variables = localSurvey.variables ?? []; | ||
| const questions = localSurvey.questions; | ||
|
|
||
| const hiddenFieldsOptions = hiddenFields.map((field) => { | ||
| return { | ||
| icon: EyeOffIcon, | ||
| label: field, | ||
| value: field, | ||
| meta: { | ||
| type: "hiddenField", | ||
| }, | ||
| }; | ||
| }); | ||
|
|
||
| const selectedVariable = variables.find((variable) => variable.id === variableId); | ||
|
|
||
| variables = variables.filter((variable) => variable.id !== variableId); | ||
|
|
||
| if (!selectedVariable) return []; | ||
|
|
||
| if (selectedVariable.type === "text") { | ||
| const allowedQuestions = questions.filter((question) => | ||
| [ | ||
| TSurveyQuestionTypeEnum.OpenText, | ||
| TSurveyQuestionTypeEnum.MultipleChoiceSingle, | ||
| TSurveyQuestionTypeEnum.Rating, | ||
| TSurveyQuestionTypeEnum.NPS, | ||
| TSurveyQuestionTypeEnum.Date, | ||
| ].includes(question.type) | ||
| ); | ||
|
|
||
| const questionOptions = allowedQuestions.map((question) => { | ||
| return { | ||
| icon: questionIconMapping[question.type], | ||
| label: getLocalizedValue(question.headline, "default"), | ||
| value: question.id, | ||
| meta: { | ||
| type: "question", | ||
| }, | ||
| }; | ||
| }); | ||
|
|
||
| const stringVariables = variables.filter((variable) => variable.type === "text"); | ||
|
|
||
| const variableOptions = stringVariables.map((variable) => { | ||
| return { | ||
| icon: FileType2Icon, | ||
| label: variable.name, | ||
| value: variable.id, | ||
| meta: { | ||
| type: "variable", | ||
| }, | ||
| }; | ||
| }); | ||
|
|
||
| const groupedOptions: TComboboxGroupedOption[] = []; | ||
|
|
||
| if (questionOptions.length > 0) { | ||
| groupedOptions.push({ | ||
| label: "Questions", | ||
| value: "questions", | ||
| options: questionOptions, | ||
| }); | ||
| } | ||
|
|
||
| if (variableOptions.length > 0) { | ||
| groupedOptions.push({ | ||
| label: "Variables", | ||
| value: "variables", | ||
| options: variableOptions, | ||
| }); | ||
| } | ||
|
|
||
| if (hiddenFieldsOptions.length > 0) { | ||
| groupedOptions.push({ | ||
| label: "Hidden Fields", | ||
| value: "hiddenFields", | ||
| options: hiddenFieldsOptions, | ||
| }); | ||
| } | ||
|
|
||
| return groupedOptions; | ||
| } else if (selectedVariable.type === "number") { | ||
| const allowedQuestions = questions.filter((question) => | ||
| [TSurveyQuestionTypeEnum.Rating, TSurveyQuestionTypeEnum.NPS].includes(question.type) | ||
| ); | ||
|
|
||
| const questionOptions = allowedQuestions.map((question) => { | ||
| return { | ||
| icon: questionIconMapping[question.type], | ||
| label: getLocalizedValue(question.headline, "default"), | ||
| value: question.id, | ||
| meta: { | ||
| type: "question", | ||
| }, | ||
| }; | ||
| }); | ||
|
|
||
| const numberVariables = variables.filter((variable) => variable.type === "number"); | ||
|
|
||
| const variableOptions = numberVariables.map((variable) => { | ||
| return { | ||
| icon: FileDigitIcon, | ||
| label: variable.name, | ||
| value: variable.id, | ||
| meta: { | ||
| type: "variable", | ||
| }, | ||
| }; | ||
| }); | ||
|
|
||
| const groupedOptions: TComboboxGroupedOption[] = []; | ||
|
|
||
| if (questionOptions.length > 0) { | ||
| groupedOptions.push({ | ||
| label: "Questions", | ||
| value: "questions", | ||
| options: questionOptions, | ||
| }); | ||
| } | ||
|
|
||
| if (variableOptions.length > 0) { | ||
| groupedOptions.push({ | ||
| label: "Variables", | ||
| value: "variables", | ||
| options: variableOptions, | ||
| }); | ||
| } | ||
|
|
||
| if (hiddenFieldsOptions.length > 0) { | ||
| groupedOptions.push({ | ||
| label: "Hidden Fields", | ||
| value: "hiddenFields", | ||
| options: hiddenFieldsOptions, | ||
| }); | ||
| } | ||
|
|
There was a problem hiding this comment.
Ensure selectedVariable is defined before proceeding
In the getActionValueOptions function, if selectedVariable is not found (i.e., it is undefined), the function returns an empty array. However, the code continues to process selectedVariable afterward, which could lead to runtime errors.
Consider adding an early return after checking if selectedVariable is undefined to prevent further execution:
const selectedVariable = variables.find((variable) => variable.id === variableId);
variables = variables.filter((variable) => variable.id !== variableId);
if (!selectedVariable) return [];
+ // Ensure selectedVariable is defined before proceedingCommittable suggestion was skipped due to low confidence.
mattinannt
dismissed
Dhruwang’s stale review
requested changes have been addressed
5 participants
What does this PR do?
This pull request includes significant updates to the logic editor documentation, image format changes, navigation updates, survey template refactoring, and component imports. The most important changes include adding a new Logic Editor documentation page, converting image formats from PNG to WEBP, updating the navigation to include the new Logic Editor, refactoring survey templates to use functions, and updating the
AdvancedSettingscomponent to useConditionalLogic.Documentation Updates:
apps/docs/app/global/logic-editor/page.mdx: Added a comprehensive documentation page for the Logic Editor, including images and detailed instructions on creating and managing logic blocks.Image Format Changes:
apps/docs/app/self-hosting/configuration/page.mdx: Converted image formats from PNG to WEBP for better performance.Navigation Updates:
apps/docs/lib/navigation.ts: Added a new navigation link for the Logic Editor documentation.Survey Template Refactoring:
apps/web/app/(app)/(onboarding)/environments/[environmentId]/xm-templates/lib/xm-templates.ts: Refactored survey templates (NPSSurvey,StarRatingSurvey,CSATSurvey,CESSurvey,SmileysRatingSurvey,eNPSSurvey) to use functions, improving code maintainability and reusability. (apps/web/app/(app)/(onboarding)/environments/[environmentId]/xm-templates/lib/xm-templates.tsL15-R16, apps/web/app/(app)/(onboarding)/environments/[environmentId]/xm-templates/lib/xm-templates.tsR45-R86, apps/web/app/(app)/(onboarding)/environments/[environmentId]/xm-templates/lib/xm-templates.tsL62-R132, apps/web/app/(app)/(onboarding)/environments/[environmentId]/xm-templates/lib/xm-templates.tsR143-R184, apps/web/app/(app)/(onboarding)/environments/[environmentId]/xm-templates/lib/xm-templates.tsL102-R228, apps/web/app/(app)/(onboarding)/environments/[environmentId]/xm-templates/lib/xm-templates.tsR237-R240, apps/web/app/(app)/(onboarding)/environments/[environmentId]/xm-templates/lib/xm-templates.tsR265-R306, apps/web/app/(app)/(onboarding)/environments/[environmentId]/xm-templates/lib/xm-templates.tsL164-R352, apps/web/app/(app)/(onboarding)/environments/[environmentId]/xm-templates/lib/xm-templates.tsR363-R366, apps/web/app/(app)/(onboarding)/environments/[environmentId]/xm-templates/lib/xm-templates.tsR397-R405)Component Import Updates:
apps/web/app/(app)/(survey-editor)/environments/[environmentId]/surveys/[surveyId]/edit/components/AdvancedSettings.tsx: Updated theAdvancedSettingscomponent to useConditionalLogicinstead ofLogicEditor. (apps/web/app/(app)/(survey-editor)/environments/[environmentId]/surveys/[surveyId]/edit/components/AdvancedSettings.tsxR1-L3, apps/web/app/(app)/(survey-editor)/environments/[environmentId]/surveys/[surveyId]/edit/components/AdvancedSettings.tsxL22-L31)Fixes
#3059
https://github.com/formbricks/internal/issues/273
https://github.com/formbricks/internal/issues/312
and also #2691