packages/surveys/src/lib/utils.ts (2)

22-34: LGTM: New getShuffledRows function implemented correctly.

The new getShuffledRows function correctly implements the shuffling logic based on the provided shuffleOption. It handles both "all" and "exceptLast" cases as expected.

Consider extracting the shuffling logic for the "exceptLast" case into a separate function for improved readability:

const shuffleExceptLast = (array: any[]) => {
  const lastElement = array.pop();
  if (lastElement) {
    shuffle(array);
    array.push(lastElement);
  }
};

export const getShuffledRows = (rows: TI18nString[], shuffleOption: TShuffleOption): TI18nString[] => {
  const rowsCopy = [...rows];
  if (shuffleOption === "all") {
    shuffle(rowsCopy);
  } else if (shuffleOption === "exceptLast") {
    shuffleExceptLast(rowsCopy);
  }
  return rowsCopy;
};

This change would make the main function more concise and easier to read.

---

Line range hint 36-59: LGTM: getShuffledChoicesIds function updated correctly.

The modifications to the getShuffledChoicesIds function correctly implement the new shuffling logic using the TShuffleOption type. The function maintains proper handling of the "other" option and correctly implements both "all" and "exceptLast" shuffling options.

While reviewing this function, I noticed a minor typo in the calculateElementIdx function (which is not part of the changes in this PR). The parameter currentQustionIdx should be currentQuestionIdx. Consider fixing this in a future PR or as a separate commit.

packages/surveys/src/components/questions/MatrixQuestion.tsx (1)

46-51: Approve with suggestions: Row shuffling implementation using useMemo.

The useMemo hook correctly implements the row shuffling functionality based on the shuffleOption. However, there are a couple of points to consider:

1. The dependency array includes question.rows[question.rows.length - 1].id, which might cause unnecessary re-renders if the last row's ID changes frequently. Consider using a more stable dependency, such as a unique identifier for the entire question or the shuffleOption itself.

2. The eslint-disable comment suggests there might be a linting issue with the dependencies. It's worth reviewing if all necessary dependencies are included without over-specifying.

Consider refactoring the dependency array to:

}, [question.shuffleOption, question.id]);

This change assumes that question.id is a stable identifier for the question and its rows. If you need to react to changes in the rows themselves, consider adding a separate rowVersion or lastUpdated field to the question object that you can use in the dependency array.

apps/web/app/(app)/(survey-editor)/environments/[environmentId]/surveys/[surveyId]/edit/components/MatrixQuestionForm.tsx (2)

88-104: LGTM: shuffleOptionsTypes object well-structured.

The shuffleOptionsTypes object is clear and aligns with the PR objective. However, consider the necessity of the 'show' property, as it's always set to true.

If the 'show' property isn't used elsewhere in the codebase, consider simplifying the object structure:

const shuffleOptionsTypes = {
  none: {
    id: "none",
    label: "Keep current order",
  },
  all: {
    id: "all",
    label: "Randomize all",
  },
  exceptLast: {
    id: "exceptLast",
    label: "Randomize all except last option",
  },
};

---

239-263: LGTM: Shuffle options Select component implemented correctly.

The Select component for shuffle options is well-implemented and functions as expected. However, consider the placement of this component within the form.

The shuffle options Select is currently placed at the end of the form, which might not be immediately visible to users. Consider moving it to a more prominent position, such as near the top of the form or just below the question headline. This could improve user experience by making the shuffle option more discoverable.

For example, you could place it after the question headline:

<QuestionFormInput
  id="headline"
  value={question.headline}
  label={"Question*"}
  // ... other props ...
/>

<div className="mt-3">
  <Label htmlFor="shuffleOption">Row Order</Label>
  <Select
    defaultValue={question.shuffleOption}
    value={question.shuffleOption}
    onValueChange={(e: TShuffleOption) => {
      updateQuestion(questionIdx, { shuffleOption: e });
    }}>
    {/* ... Select content ... */}
  </Select>
</div>

{/* ... rest of the form ... */}

This placement would make the shuffle option more visible and accessible to users.

Configuration used: CodeRabbit UI
Review profile: CHILL

packages/ui/components/ShuffleOptionSelect/index.tsx (1)

37-68: LGTM: Component implementation is well-structured. Consider adding error handling.

The ShuffleOptionSelect component is well-implemented, following React best practices. The use of destructured props and efficient rendering of SelectItem components are commendable.

A minor suggestion for improvement:

Consider adding error handling or a fallback UI for cases where shuffleOptionsTypes might be empty or undefined. This could prevent potential runtime errors and improve the component's robustness.

Example:

{Object.values(shuffleOptionsTypes).length > 0 ? (
  Object.values(shuffleOptionsTypes).map(
    // ... existing code
  )
) : (
  <SelectItem value="none" disabled>No options available</SelectItem>
)}

This change ensures that the component always renders something, even if there are no shuffle options available.

packages/surveys/src/lib/utils.ts (4)

21-35: LGTM: New function getRandomRowIdx implemented correctly.

The function successfully implements the shuffling logic for both "all" and "exceptLast" options, aligning with the PR objective. It correctly uses the existing shuffle function and handles the "exceptLast" case properly.

Consider adding input validation for the n parameter to ensure it's a positive integer:

if (n <= 0 || !Number.isInteger(n)) {
  throw new Error("n must be a positive integer");
}

---

37-49: LGTM: New function getShuffledRows implemented correctly.

The function successfully implements the shuffling logic for both "all" and "exceptLast" options, aligning with the PR objective. It correctly uses the existing shuffle function and handles the "exceptLast" case properly.

Consider refactoring to reuse the logic from getRandomRowIdx:

export const getShuffledRows = (rows: TI18nString[], shuffleOption: TShuffleOption): TI18nString[] => {
  const shuffledIndices = getRandomRowIdx(rows.length, shuffleOption);
  return shuffledIndices.map(index => rows[index]);
};

This refactoring would reduce code duplication and improve maintainability.

---

51-54: LGTM: Function getShuffledChoicesIds updated correctly.

The function has been successfully updated to use the new TShuffleOption type and implement the shuffling logic for both "all" and "exceptLast" options. It maintains the existing functionality for handling the "other" option.

For consistency with getShuffledRows, consider using the spread operator when creating shuffledChoices:

const shuffledChoices = otherOption ? choices.filter((choice) => choice.id !== "other") : [...choices];

This change would make the code more consistent across similar functions.

---

Remaining usages of the old shuffle method detected.

The new shuffling functions (getRandomRowIdx, getShuffledRows, and getShuffledChoicesIds) are correctly integrated and used in the codebase. However, the old shuffle method is still being utilized within utils.ts. Please refactor the code to replace all instances of the old shuffle function with the new shuffling functions to maintain consistency and improve code maintainability.

🔗 Analysis chain

Line range hint 1-124: Overall implementation looks good, with room for minor improvements.

The changes successfully implement the row randomization feature for matrix questions, aligning with the PR objective. The new functions and modifications to existing ones are consistent with the codebase's style and structure.

Consider grouping related functions together for better code organization. For example, you could place getRandomRowIdx, getShuffledRows, and getShuffledChoicesIds in a section dedicated to shuffling operations. This would improve the file's readability and make it easier to maintain related functionality.

To ensure that the changes don't introduce any unintended side effects, please run the following verification script:

This script will help identify any places where the new functions are used and ensure that the old shuffling method with a string option is no longer in use.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify that the new shuffling functions are used correctly throughout the codebase.

# Test: Search for usages of the new and modified functions
echo "Searching for usages of getRandomRowIdx:"
rg --type typescript 'getRandomRowIdx'

echo "Searching for usages of getShuffledRows:"
rg --type typescript 'getShuffledRows'

echo "Searching for usages of getShuffledChoicesIds:"
rg --type typescript 'getShuffledChoicesIds'

# Test: Check for any remaining usages of the old shuffling method
echo "Checking for any remaining usages of the old shuffling method:"
rg --type typescript 'shuffle.*option.*string'

Length of output: 731

---

Script:

#!/bin/bash
# Description: Verify that the new shuffling functions are used correctly throughout the codebase.

# Test: Search for usages of the new and modified functions within .ts and .tsx files
echo "Searching for usages of getRandomRowIdx:"
rg --glob '*.ts' --glob '*.tsx' 'getRandomRowIdx'

echo "Searching for usages of getShuffledRows:"
rg --glob '*.ts' --glob '*.tsx' 'getShuffledRows'

echo "Searching for usages of getShuffledChoicesIds:"
rg --glob '*.ts' --glob '*.tsx' 'getShuffledChoicesIds'

# Test: Check for any remaining usages of the old shuffling method without relying on file type
echo "Checking for any remaining usages of the old shuffling method:"
rg 'shuffle\([^)]*\)' --glob '*.ts' --glob '*.tsx'

Length of output: 2196

apps/web/app/(app)/(survey-editor)/environments/[environmentId]/surveys/[surveyId]/edit/components/RankingQuestionForm.tsx (2)

219-224: LGTM: ShuffleOptionSelect implementation looks good

The addition of the ShuffleOptionSelect component successfully implements the randomization feature for ranking questions, aligning with the PR objectives. The reuse of existing shuffle option types maintains consistency across the application.

Consider adding a comment explaining the purpose of the ShuffleOptionSelect component for better code readability.

You could add a comment above the ShuffleOptionSelect component to explain its purpose:

+            {/* ShuffleOptionSelect allows users to choose how to randomize the order of ranking options */}
            <ShuffleOptionSelect
              shuffleOptionsTypes={shuffleOptionsTypes}
              updateQuestion={updateQuestion}
              shuffleOption={question.shuffleOption}
              questionIdx={questionIdx}
            />

---

Line range hint 1-230: Consider extracting shuffleOptionsTypes for better reusability

The overall implementation of the RankingQuestionForm component is well-structured and successfully integrates the new randomization feature while preserving existing functionality. However, to improve code organization and reusability, consider extracting the shuffleOptionsTypes object to a separate file or a higher-level component.

You could create a new file, e.g., shuffleOptions.ts, and move the shuffleOptionsTypes definition there:

// shuffleOptions.ts
export const shuffleOptionsTypes = {
  none: {
    id: "none",
    label: "Keep current order",
    show: true,
  },
  all: {
    id: "all",
    label: "Randomize all",
    show: true, // You might want to make this dynamic based on the context
  },
};

Then, import and use it in the RankingQuestionForm component:

import { shuffleOptionsTypes } from './shuffleOptions';

// ... rest of the component code

// Use shuffleOptionsTypes directly in the ShuffleOptionSelect component
<ShuffleOptionSelect
  shuffleOptionsTypes={shuffleOptionsTypes}
  // ... other props
/>

This change would make the shuffle options more reusable across different components and easier to maintain.

apps/web/app/(app)/(survey-editor)/environments/[environmentId]/surveys/[surveyId]/edit/components/MultipleChoiceQuestionForm.tsx (1)

Line range hint 54-69: LGTM: Enhanced shuffle options with improved flexibility.

The updates to the shuffleOptionsTypes object provide more flexibility in randomization options, which aligns well with the PR objectives. The condition for showing the "all" option based on the absence of an "other" choice is a good safeguard against potential conflicts.

Consider extracting the shuffleOptionsTypes object to a separate constant or configuration file to improve code organization and maintainability, especially if these options might be reused elsewhere in the application.

packages/types/surveys/types.ts (2)

576-576: LGTM! Consider adding a comment for clarity.

The addition of the shuffleOption property to ZSurveyMatrixQuestion is well-implemented and aligns with the PR objective. It uses the existing ZShuffleOption type and provides a sensible default value, maintaining backwards compatibility.

Consider adding a brief comment explaining the purpose of the shuffleOption property, e.g.:

// Determines how the rows of the matrix question should be shuffled (if at all)
shuffleOption: ZShuffleOption.optional().default("none"),

---

Line range hint 1-1024: Overall, the change is well-implemented and consistent. Consider standardizing shuffle options across question types.

The addition of the shuffleOption to ZSurveyMatrixQuestion is isolated and doesn't negatively impact the rest of the file. It's consistent with similar implementations in ZSurveyMultipleChoiceQuestion and ZSurveyRankingQuestion.

For improved consistency and maintainability, consider creating a reusable interface or type for questions that support shuffling. This could be applied to ZSurveyMatrixQuestion, ZSurveyMultipleChoiceQuestion, and ZSurveyRankingQuestion. For example:

interface IShuffleable {
  shuffleOption: z.ZodOptional<typeof ZShuffleOption>;
}

// Then use it in the question types:
export const ZSurveyMatrixQuestion = ZSurveyQuestionBase.extend({
  // ... other properties
}).extend<IShuffleable>({
  shuffleOption: ZShuffleOption.optional().default("none"),
});

This approach would make it easier to manage and extend shuffle functionality across different question types in the future.

Configuration used: CodeRabbit UI
Review profile: CHILL

packages/ui/components/ShuffleOptionSelect/index.tsx (2)

21-29: Well-defined interface with a minor improvement suggestion.

The ShuffleOptionSelectInterface is comprehensive and includes all necessary properties for the component. However, to improve readability and maintainability, consider extracting the updateQuestion function type into a separate type definition.

Consider refactoring the updateQuestion function type:

type UpdateQuestionFunction = (
  questionIdx: number,
  updatedAttributes: Partial<TSurveyMatrixQuestion | TSurveyMultipleChoiceQuestion | TSurveyRankingQuestion>
) => void;

interface ShuffleOptionSelectInterface {
  shuffleOption: TShuffleOption | undefined;
  updateQuestion: UpdateQuestionFunction;
  questionIdx: number;
  shuffleOptionsTypes: ShuffleOptionsTypes;
}

This change would make the interface more readable and easier to maintain.

---

31-62: Well-implemented component with a minor suggestion.

The ShuffleOptionSelect component is well-structured and follows React best practices. It effectively uses the Select component from the UI library and correctly implements the shuffle option selection functionality.

Consider adding a default placeholder text for better user experience:

- <SelectValue placeholder="Select ordering" />
+ <SelectValue placeholder={shuffleOption ? undefined : "Select ordering"} />

This change ensures that the placeholder is only shown when no option is selected, improving the component's usability.

Configuration used: CodeRabbit UI
Review profile: CHILL

packages/ui/components/ShuffleOptionSelect/index.tsx (1)

31-62: Well-implemented component with a minor suggestion

The ShuffleOptionSelect component is well-structured and follows React best practices. The use of the Select component from your UI library promotes consistency, and the conditional rendering of SelectItem components is a good approach.

One minor suggestion for improved type safety:

onValueChange={(e: TShuffleOption) => {
  updateQuestion(questionIdx, { shuffleOption: e });
}}

Consider using a more specific type for e instead of TShuffleOption. Since shuffleOptionsTypes defines the possible values, you could use:

onValueChange={(e: keyof ShuffleOptionsTypes) => {
  updateQuestion(questionIdx, { shuffleOption: e });
}}

This ensures that only valid shuffle options can be selected.

packages/surveys/src/lib/utils.ts (2)

21-35: LGTM: New function implements shuffling logic correctly.

The getShuffledRowIndices function correctly implements the shuffling logic based on the provided option, handling both "all" and "exceptLast" cases as required. This aligns well with the PR objectives for randomizing row order in matrix questions.

Consider adding a check for edge cases where n is 0 or negative to ensure robust behavior:

 export const getShuffledRowIndices = (n: number, shuffleOption: TShuffleOption): number[] => {
+  if (n <= 0) return [];
   // Rest of the function remains the same
 };

---

37-40: LGTM: Function updated to use new shuffling logic.

The getShuffledChoicesIds function has been successfully updated to use the TShuffleOption type and implement the new shuffling logic. The changes are consistent with the PR objectives and maintain the existing functionality for handling the "other" option.

Consider refactoring to reuse logic from getShuffledRowIndices for consistency and maintainability:

export const getShuffledChoicesIds = (
  choices: TSurveyQuestionChoice[],
  shuffleOption: TShuffleOption
): string[] => {
  const otherOption = choices.find((choice) => choice.id === "other");
  const choicesWithoutOther = otherOption ? choices.filter((choice) => choice.id !== "other") : choices;
  
  const shuffledIndices = getShuffledRowIndices(choicesWithoutOther.length, shuffleOption);
  const shuffledChoices = shuffledIndices.map(index => choicesWithoutOther[index]);
  
  if (otherOption) shuffledChoices.push(otherOption);
  
  return shuffledChoices.map((choice) => choice.id);
};

This refactoring would centralize the shuffling logic in getShuffledRowIndices, making the code more DRY and easier to maintain.

apps/web/app/(app)/(survey-editor)/environments/[environmentId]/surveys/[surveyId]/edit/components/MatrixQuestionForm.tsx (2)

82-98: LGTM with suggestions: shuffleOptionsTypes object

The shuffleOptionsTypes object is well-structured and covers the main scenarios for shuffling options. However, consider the following suggestions:

1. If the 'show' property is always true, you might want to remove it to simplify the object structure.
2. If this object is or will be used in multiple components, consider moving it to a separate constants file for better maintainability.

Example of simplified structure:

const shuffleOptionsTypes = {
  none: { id: "none", label: "Keep current order" },
  all: { id: "all", label: "Randomize all" },
  exceptLast: { id: "exceptLast", label: "Randomize all except last option" },
};

---

233-240: LGTM with suggestion: ShuffleOptionSelect implementation

The ShuffleOptionSelect component is correctly implemented and placed within the form structure. All necessary props are passed, which aligns with the PR objectives to allow users to randomize the order of rows in matrix questions.

Suggestion: Consider adding a brief comment above this section to explain its purpose and how it relates to the matrix question functionality. This will help other developers understand the context quickly.

Example:

{/* Shuffle options for randomizing matrix question rows */}
<div className="mt-3 flex flex-1 items-center justify-end gap-2">
  <ShuffleOptionSelect
    // ... (rest of the code)
  />
</div>

Configuration used: CodeRabbit UI
Review profile: CHILL

Configuration used: CodeRabbit UI
Review profile: CHILL