🐛 Use natural sort order (#1918)

Replace default alphabetical sort order (i.e. a, a11, a2)               
with human friendly "natural" sort (i.e. a, a2, a11).                   
The change applies to all found cases of string sorting.                
For convenience the comparator converts all values to strings (with     
nullish values being converted to empty string).                         
                                                                        
In addition, the natural sort with string conversion (universal
comparator) was used as a replacement in sorting hooks. There a general 
sorting algorithm was implemented with type-specific comparators.     
Benefits of universal comparator:                                       
1. better handling of mixed types (i.e. string and number). String value
   most likely gets converted to NaN.                                   
2. better handling of undefined (numeric value NaN)                     
2. simplified coercion logic compared to less than/greater then         
   operators 

Note that one functional change was introduced in the sorting hook:
space removal logic was dropped as it creates visually confusing results
for stable sorting.

Resolves: #1796 

Reference-Url:
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/Less_than

---------

Signed-off-by: Radoslaw Szwajkowski <[email protected]>

client/src/app/hooks/table-controls/sorting/getLocalSortDerivedState.ts

@@ -1,5 +1,6 @@
 import i18n from "@app/i18n";
 import { ISortState } from "./useSortState";
+import { universalComparator } from "@app/utils/utils";
 
 /**
  * Args for getLocalSortDerivedState
@@ -51,22 +52,10 @@ export const getLocalSortDerivedState = <
 
   let sortedItems = items;
   sortedItems = [...items].sort((a: TItem, b: TItem) => {
-    let aValue = getSortValues(a)[activeSort.columnKey];
-    let bValue = getSortValues(b)[activeSort.columnKey];
-    if (typeof aValue === "string" && typeof bValue === "string") {
-      aValue = aValue.replace(/ +/g, "");
-      bValue = bValue.replace(/ +/g, "");
-      const aSortResult = aValue.localeCompare(bValue, i18n.language);
-      const bSortResult = bValue.localeCompare(aValue, i18n.language);
-      return activeSort.direction === "asc" ? aSortResult : bSortResult;
-    } else if (typeof aValue === "number" && typeof bValue === "number") {
-      return activeSort.direction === "asc" ? aValue - bValue : bValue - aValue;
-    } else {
-      if (aValue > bValue) return activeSort.direction === "asc" ? -1 : 1;
-      if (aValue < bValue) return activeSort.direction === "asc" ? -1 : 1;
-    }
-
-    return 0;
+    const aValue = getSortValues(a)[activeSort.columnKey];
+    const bValue = getSortValues(b)[activeSort.columnKey];
+    const compareValue = universalComparator(aValue, bValue, i18n.language);
+    return activeSort.direction === "asc" ? compareValue : -compareValue;
   });
 
   return { sortedItems };

client/src/app/hooks/useLegacySortState.ts

@@ -1,6 +1,7 @@
 import * as React from "react";
 import { ISortBy, SortByDirection } from "@patternfly/react-table";
 import i18n from "@app/i18n";
+import { universalComparator } from "@app/utils/utils";
 
 /**
  * @deprecated The return value of useLegacySortState which predates table-controls/table-batteries and is deprecated.
@@ -49,27 +50,10 @@ export const useLegacySortState = <T>(
   if (sortBy.index !== undefined && sortBy.direction !== undefined) {
     sortedItems = [...items].sort((a: T, b: T) => {
       const { index, direction } = sortBy;
-      let aValue = getSortValues(a)[index || 0];
-      let bValue = getSortValues(b)[index || 0];
-      if (typeof aValue === "string" && typeof bValue === "string") {
-        aValue = aValue.replace(/ +/g, "");
-        bValue = bValue.replace(/ +/g, "");
-        const aSortResult = aValue.localeCompare(bValue, i18n.language);
-        const bSortResult = bValue.localeCompare(aValue, i18n.language);
-        if (direction === "asc") {
-          return aSortResult;
-        } else {
-          return bSortResult;
-        }
-      } else if (typeof aValue === "number" && typeof bValue === "number") {
-        if (direction === "asc") return aValue - bValue;
-        else return bValue - aValue;
-      } else {
-        if (aValue > bValue) return direction === "asc" ? -1 : 1;
-        if (aValue < bValue) return direction === "asc" ? -1 : 1;
-      }
-
-      return 0;
+      const aValue = getSortValues(a)[index || 0];
+      const bValue = getSortValues(b)[index || 0];
+      const compareValue = universalComparator(aValue, bValue, i18n.language);
+      return direction === "asc" ? compareValue : -compareValue;
     });
   }
 

client/src/app/pages/applications/analysis-wizard/set-options.tsx

@@ -18,7 +18,7 @@ import { useFormContext } from "react-hook-form";
 import { useTranslation } from "react-i18next";
 import * as yup from "yup";
 
-import { getValidatedFromErrors } from "@app/utils/utils";
+import { getValidatedFromErrors, universalComparator } from "@app/utils/utils";
 import { defaultTargets } from "../../../data/targets";
 import spacing from "@patternfly/react-styles/css/utilities/Spacing/spacing";
 import { AnalysisWizardFormValues } from "./schema";
@@ -71,11 +71,7 @@ export const SetOptions: React.FC = () => {
   const defaultTargetsAndTargetsLabels = [
     ...defaultTargets,
     ...allTargetLabelsFromTargets,
-  ].sort((t1, t2) => {
-    if (t1.label > t2.label) return 1;
-    if (t1.label < t2.label) return -1;
-    return 0;
-  });
+  ].sort((t1, t2) => universalComparator(t1.label, t2.label));
 
   const defaultSourcesAndSourcesLabels = [
     ...new Set(defaultSources.concat(allSourceLabelsFromTargets)),

client/src/app/pages/applications/components/application-tags/application-tags.tsx

@@ -29,18 +29,14 @@ import {
 import { useLegacyFilterState } from "@app/hooks/useLegacyFilterState";
 import { useHistory } from "react-router-dom";
 import { ItemTagLabel } from "../../../../components/labels/item-tag-label/item-tag-label";
-import { capitalizeFirstLetter } from "@app/utils/utils";
+import { capitalizeFirstLetter, universalComparator } from "@app/utils/utils";
 
 interface TagWithSource extends Tag {
   source?: string;
 }
 
-const compareSources = (a: string, b: string) => {
-  // Always put Manual tags (source === "") first
-  if (a === "") return -1;
-  if (b === "") return 1;
-  return a.localeCompare(b);
-};
+// Always put Manual tags (source === "") first
+const compareSources = (a: string, b: string) => universalComparator(a, b);
 
 export interface ApplicationTagsProps {
   application: Application;
@@ -152,7 +148,7 @@ export const ApplicationTags: React.FC<ApplicationTagsProps> = ({
       getItemValue: (tag) => tag.category?.name || "",
       selectOptions: Array.from(tagCategoriesById.values())
         .map((tagCategory) => tagCategory.name)
-        .sort((a, b) => a.localeCompare(b))
+        .sort(universalComparator)
         .map((tagCategoryName) => ({
           key: tagCategoryName,
           value: tagCategoryName,

client/src/app/pages/archetypes/components/archetype-form/archetype-form.tsx

@@ -23,7 +23,11 @@ import {
   useCreateArchetypeMutation,
   useUpdateArchetypeMutation,
 } from "@app/queries/archetypes";
-import { duplicateNameCheck, getAxiosErrorMessage } from "@app/utils/utils";
+import {
+  duplicateNameCheck,
+  getAxiosErrorMessage,
+  universalComparator,
+} from "@app/utils/utils";
 import { type TagItemType, useFetchTagsWithTagItems } from "@app/queries/tags";
 
 import { useFetchStakeholderGroups } from "@app/queries/stakeholdergroups";
@@ -198,8 +202,14 @@ const ArchetypeForm: React.FC<ArchetypeFormProps> = ({
         .map(({ id }) => tagItems.find((tag) => tag.id === id))
         .filter(Boolean),
 
-      stakeholders: archetype?.stakeholders?.sort() ?? [],
-      stakeholderGroups: archetype?.stakeholderGroups?.sort() ?? [],
+      stakeholders:
+        archetype?.stakeholders?.sort((a, b) =>
+          universalComparator(a.name, b.name)
+        ) ?? [],
+      stakeholderGroups:
+        archetype?.stakeholderGroups?.sort((a, b) =>
+          universalComparator(a.name, b.name)
+        ) ?? [],
     },
     resolver: yupResolver(validationSchema),
     mode: "all",

client/src/app/pages/controls/tags/components/tag-form.tsx

@@ -11,7 +11,7 @@ import {
 
 import { DEFAULT_SELECT_MAX_HEIGHT } from "@app/Constants";
 import { New, Tag, TagCategory } from "@app/api/models";
-import { duplicateNameCheck } from "@app/utils/utils";
+import { duplicateNameCheck, universalComparator } from "@app/utils/utils";
 import { ITagCategoryDropdown } from "@app/utils/model-utils";
 import {
   useFetchTags,
@@ -56,7 +56,7 @@ export const TagForm: React.FC<TagFormProps> = ({ tag, onClose }) => {
       };
     });
 
-    return options.sort((a, b) => a.value.localeCompare(b.value));
+    return options.sort((a, b) => universalComparator(a.value, b.value));
   }, [tagCategories]);
 
   const tagCategoryInitialValue: ITagCategoryDropdown | null = useMemo(() => {

client/src/app/pages/controls/tags/components/tag-table.tsx

@@ -11,6 +11,7 @@ import {
 } from "@patternfly/react-table";
 import { Tag, TagCategory } from "@app/api/models";
 import "./tag-table.css";
+import { universalComparator } from "@app/utils/utils";
 
 export interface TabTableProps {
   tagCategory: TagCategory;
@@ -35,7 +36,7 @@ export const TagTable: React.FC<TabTableProps> = ({
       </Thead>
       <Tbody>
         {(tagCategory.tags || [])
-          .sort((a, b) => a.name.localeCompare(b.name))
+          .sort((a, b) => universalComparator(a.name, b.name))
           .map((tag) => (
             <Tr key={tag.name}>
               <Td>{tag.name}</Td>

client/src/app/pages/controls/tags/tags.tsx

@@ -29,7 +29,7 @@ import { CubesIcon } from "@patternfly/react-icons";
 import {
   dedupeFunction,
   getAxiosErrorMessage,
-  localeNumericCompare,
+  universalComparator,
 } from "@app/utils/utils";
 import { Tag, TagCategory } from "@app/api/models";
 import { FilterToolbar, FilterType } from "@app/components/FilterToolbar";
@@ -57,7 +57,6 @@ import {
 import { useLocalTableControls } from "@app/hooks/table-controls";
 import { RBAC, controlsWriteScopes, RBAC_TYPE } from "@app/rbac";
 import { TagTable } from "./components/tag-table";
-import i18n from "@app/i18n";
 
 export const Tags: React.FC = () => {
   const { t } = useTranslation();
@@ -204,9 +203,7 @@ export const Tags: React.FC = () => {
                 value: tagCategory?.name,
               })) ?? []
             )
-            .sort((a, b) =>
-              localeNumericCompare(a.value, b.value, i18n.language)
-            )
+            .sort((a, b) => universalComparator(a.value, b.value))
         ),
       },
       {

client/src/app/pages/issues/helpers.ts

@@ -23,8 +23,7 @@ import { useFetchTagsWithTagItems } from "@app/queries/tags";
 import { useTranslation } from "react-i18next";
 import { useFetchArchetypes } from "@app/queries/archetypes";
 import { useFetchApplications } from "@app/queries/applications";
-import { localeNumericCompare } from "@app/utils/utils";
-import i18n from "@app/i18n";
+import { universalComparator } from "@app/utils/utils";
 
 // Certain filters are shared between the Issues page and the Affected Applications Page.
 // We carry these filter values between the two pages when determining the URLs to navigate between them.
@@ -58,7 +57,7 @@ export const useSharedAffectedApplicationFilterCategories = <
         }) + "...",
       selectOptions: applications
         .map(({ name }) => name)
-        .sort((a, b) => localeNumericCompare(a, b, i18n.language))
+        .sort(universalComparator)
         .map((name) => ({
           key: name,
           value: name,

client/src/app/pages/reports/reports.tsx

@@ -30,6 +30,7 @@ import { ApplicationSelectionContextProvider } from "./application-selection-con
 import { IdentifiedRisksTable } from "./components/identified-risks-table";
 import { toIdRef } from "@app/utils/model-utils";
 import { ApplicationLandscape } from "./components/application-landscape";
+import { universalComparator } from "@app/utils/utils";
 
 const ALL_QUESTIONNAIRES = -1;
 
@@ -112,7 +113,7 @@ export const Reports: React.FC = () => {
         )
           .map((id) => questionnairesById[id])
           .filter((questionnaire) => questionnaire !== undefined)
-          .sort((a, b) => a.name.localeCompare(b.name));
+          .sort((a, b) => universalComparator(a.name, b.name));
 
   const isAllQuestionnairesSelected =
     selectedQuestionnaireId === ALL_QUESTIONNAIRES;

client/src/app/queries/tags.ts

@@ -12,6 +12,7 @@ import {
   updateTagCategory,
 } from "@app/api/rest";
 import { AxiosError } from "axios";
+import { universalComparator } from "@app/utils/utils";
 
 export const TagsQueryKey = "tags";
 export const TagCategoriesQueryKey = "tagcategories";
@@ -88,7 +89,7 @@ export const useFetchTagsWithTagItems = () => {
         name: `${tag.category?.name} / ${tag.name}`,
         tooltip: tag.category?.name,
       }))
-      .sort((a, b) => a.name.localeCompare(b.name));
+      .sort((a, b) => universalComparator(a.name, b.name));
   }, [tags]);
 
   return {

client/src/app/queries/tasks.ts

@@ -1,6 +1,7 @@
 import { useMutation, useQuery } from "@tanstack/react-query";
 
 import { cancelTask, deleteTask, getTaskById, getTasks } from "@app/api/rest";
+import { universalComparator } from "@app/utils/utils";
 
 interface FetchTasksFilters {
   addon?: string;
@@ -22,15 +23,11 @@ export const useFetchTasks = (
           filters?.addon ? filters.addon === task.addon : true
         )
         // sort by application.id (ascending) then createTime (newest to oldest)
-        .sort((a, b) => {
-          if (a.application.id !== b.application.id) {
-            return a.application.id - b.application.id;
-          } else {
-            const aTime = a?.createTime ?? "";
-            const bTime = b?.createTime ?? "";
-            return aTime < bTime ? 1 : aTime > bTime ? -1 : 0;
-          }
-        })
+        .sort((a, b) =>
+          a.application.id !== b.application.id
+            ? a.application.id - b.application.id
+            : -1 * universalComparator(a.createTime, b.createTime)
+        )
         // remove old tasks for each application
         .filter(
           (task, index, tasks) =>

client/src/app/utils/utils.ts

@@ -2,6 +2,7 @@ import * as yup from "yup";
 import { AxiosError } from "axios";
 import { ToolbarChip } from "@patternfly/react-core";
 import { AdminPathValues, DevPathValues } from "@app/Paths";
+import i18n from "@app/i18n";
 
 // Axios error
 
@@ -195,8 +196,22 @@ export const capitalizeFirstLetter = (str: string) =>
 export const localeNumericCompare = (
   a: string,
   b: string,
-  locale: string
-): number => a.localeCompare(b, locale, { numeric: true });
+  locale: string = i18n.language
+): number => a.localeCompare(b, locale ?? "en", { numeric: true });
 
 export const getString = (input: string | (() => string)) =>
   typeof input === "function" ? input() : input;
+
+/**
+ * Compares all types by converting them to string.
+ * Nullish entities are converted to empty string.
+ * @see localeNumericCompare
+ * @param locale to be used by string compareFn
+ */
+export const universalComparator = (
+  // eslint-disable-next-line @typescript-eslint/no-explicit-any
+  a: any,
+  // eslint-disable-next-line @typescript-eslint/no-explicit-any
+  b: any,
+  locale: string = i18n.language
+) => localeNumericCompare(String(a ?? ""), String(b ?? ""), locale);