Address code review feedback

Issue https://gitlab.com/gitlab-org/gitlab/-/issues/324649

app/assets/javascripts/performance_bar/components/detailed_metric.vue

@@ -2,11 +2,9 @@
 import { GlButton, GlModal, GlModalDirective, GlSegmentedControl } from '@gitlab/ui';
 
 import { s__ } from '~/locale';
+import { sortOrders, sortOrderOptions } from '../constants';
 import RequestWarning from './request_warning.vue';
 
-export const SortOrderDuration = 'SortOrderDuration';
-export const SortOrderChronological = 'SortOrderChronological';
-
 export default {
   components: {
     RequestWarning,
@@ -45,7 +43,7 @@ export default {
   data() {
     return {
       openedBacktraces: [],
-      sortOrder: SortOrderDuration,
+      sortOrder: sortOrders.DURATION,
     };
   },
   computed: {
@@ -56,15 +54,11 @@ export default {
       return this.currentRequest.details[this.metric];
     },
     metricDetailsSummary() {
-      let summary = {
-        [s__('PerformanceBar|Total')]: this.metricDetails.calls,
+      return {
+        [s__('Total')]: this.metricDetails.calls,
         [s__('PerformanceBar|Total duration')]: this.metricDetails.duration,
+        ...(this.metricDetails.summary || {}),
       };
-      if (this.metricDetails.summary) {
-        summary = { ...summary, ...this.metricDetails.summary };
-      }
-
-      return summary;
     },
     metricDetailsLabel() {
       if (this.metricDetails.duration && this.metricDetails.calls) {
@@ -82,17 +76,14 @@ export default {
       );
     },
     detailsList() {
-      const list = this.metricDetails.details
-        .slice()
-        .map((item, index) => ({ ...item, id: index }));
-
-      if (this.sortOrder === SortOrderDuration) {
-        return list.sort((a, b) => (a.duration < b.duration ? 1 : -1));
-      } else if (this.sortOrder === SortOrderChronological) {
-        return list.sort((a, b) => (a.start < b.start ? -1 : 1));
+      return this.metricDetails.details.map((item, index) => ({ ...item, id: index }));
+    },
+    sortedList() {
+      if (this.sortOrder === sortOrders.CHRONOLOGICAL) {
+        return this.detailsList.slice().sort(this.sortDetailChronologically);
       }
 
-      return list;
+      return this.detailsList.slice().sort(this.sortDetailByDuration);
     },
     warnings() {
       return this.metricDetails.warnings || [];
@@ -123,11 +114,14 @@ export default {
     changeSortOrder(order) {
       this.sortOrder = order;
     },
+    sortDetailByDuration(a, b) {
+      return a.duration < b.duration ? 1 : -1;
     },
-  sortOrders: [
-    { value: SortOrderDuration, text: s__('PerformanceBar|Sort by duration') },
-    { value: SortOrderChronological, text: s__('PerformanceBar|Sort chronologically') },
-  ],
+    sortDetailChronologically(a, b) {
+      return a.start < b.start ? -1 : 1;
+    },
+  },
+  sortOrderOptions,
 };
 </script>
 <template>
@@ -148,29 +142,25 @@ export default {
     <gl-modal :modal-id="modalId" :title="header" size="lg" footer-class="d-none" scrollable>
       <div class="gl-display-flex gl-align-items-center gl-justify-content-space-between">
         <div class="gl-display-flex gl-align-items-center" data-testid="performance-bar-summary">
-          <div
-            v-for="(value, name) in metricDetailsSummary"
-            v-if="value"
-            :key="name"
-            class="gl-pr-8"
-            data-testid="performance-bar-summary-item"
-          >
+          <div v-for="(value, name) in metricDetailsSummary" :key="name" class="gl-pr-8">
+            <div v-if="value" data-testid="performance-bar-summary-item">
               <div>{{ name }}</div>
               <div class="gl-font-size-h1 gl-font-weight-bold">{{ value }}</div>
             </div>
           </div>
+        </div>
         <gl-segmented-control
           v-if="displaySortOrder"
           data-testid="performance-bar-sort-order"
-          :options="$options.sortOrders"
+          :options="$options.sortOrderOptions"
           :checked="sortOrder"
           @input="changeSortOrder"
         />
       </div>
       <hr />
       <table class="table gl-table">
-        <template v-if="detailsList.length">
-          <tr v-for="item in detailsList" :key="item.id">
+        <template v-if="sortedList.length">
+          <tr v-for="item in sortedList" :key="item.id">
             <td data-testid="performance-item-duration">
               <span v-if="item.duration">{{
                 sprintf(__('%{duration}ms'), { duration: item.duration })
@@ -198,7 +188,7 @@ export default {
                     @click="toggleBacktrace(item.id)"
                   />
                 </div>
-                <pre v-if="itemHasOpenedBacktrace(item.id)" class="backtrace-row mt-2">{{
+                <pre v-if="itemHasOpenedBacktrace(item.id)" class="backtrace-row gl-mt-3">{{
                   item.backtrace
                 }}</pre>
               </div>
@@ -207,7 +197,7 @@ export default {
         </template>
         <template v-else>
           <tr>
-            <td>
+            <td data-testid="performance-bar-empty-detail-notice">
               {{ sprintf(__('No %{header} for this request.'), { header: header.toLowerCase() }) }}
             </td>
           </tr>

app/assets/javascripts/performance_bar/components/performance_bar_app.vue

@@ -166,9 +166,9 @@ export default {
           s__('PerformanceBar|Download')
         }}</a>
       </div>
-      <div v-if="statsUrl" id="peek-stats" class="view">
-        <a class="gl-text-blue-300" :href="statsUrl">{{ s__('PerformanceBar|Stats') }}</a>
-      </div>
+      <a v-if="statsUrl" class="gl-text-blue-300 view" :href="statsUrl">{{
+        s__('PerformanceBar|Stats')
+      }}</a>
       <request-selector
         v-if="currentRequest"
         :current-request="currentRequest"

app/assets/javascripts/performance_bar/constants.js

+import { s__ } from '~/locale';
+
+export const sortOrders = {
+  DURATION: 'duration',
+  CHRONOLOGICAL: 'chronological',
+};
+
+export const sortOrderOptions = [
+  {
+    value: sortOrders.DURATION,
+    text: s__('PerformanceBar|Sort by duration'),
+  },
+  {
+    value: sortOrders.CHRONOLOGICAL,
+    text: s__('PerformanceBar|Sort chronologically'),
+  },
+];

ee/changelogs/unreleased/qmnguyen0711-improve-performance-bar.yml

 ---
-title: Improve the performance bar
+title: Allow details to be sorted chronologically in the performance bar
 merge_request: 56830
 author:
 type: changed

locale/gitlab.pot

@@ -22213,9 +22213,6 @@ msgstr ""
 msgid "PerformanceBar|Stats"
 msgstr ""
 
-msgid "PerformanceBar|Total"
-msgstr ""
-
 msgid "PerformanceBar|Total duration"
 msgstr ""
 

spec/frontend/performance_bar/components/detailed_metric_spec.js

@@ -2,35 +2,39 @@ import { shallowMount } from '@vue/test-utils';
 import { nextTick } from 'vue';
 import { trimText } from 'helpers/text_helper';
 import { extendedWrapper } from 'helpers/vue_test_utils_helper';
-import DetailedMetric, {
-  SortOrderDuration,
-  SortOrderChronological,
-} from '~/performance_bar/components/detailed_metric.vue';
+import DetailedMetric from '~/performance_bar/components/detailed_metric.vue';
 import RequestWarning from '~/performance_bar/components/request_warning.vue';
+import { sortOrders } from '~/performance_bar/constants';
 
 describe('detailedMetric', () => {
   let wrapper;
 
-  const createComponent = (props) => {
+  const defaultProps = {
+    currentRequest: {},
+    metric: 'gitaly',
+    header: 'Gitaly calls',
+    keys: ['feature', 'request'],
+  };
+
+  const createComponent = (props = {}) => {
     wrapper = extendedWrapper(
       shallowMount(DetailedMetric, {
-        propsData: {
-          ...props,
-        },
+        propsData: { ...defaultProps, ...props },
       }),
     );
   };
 
   const findAllTraceBlocks = () => wrapper.findAll('pre');
   const findTraceBlockAtIndex = (index) => findAllTraceBlocks().at(index);
-  const findExpandBacktraceBtns = () => wrapper.findAll('[data-testid="backtrace-expand-btn"]');
+  const findExpandBacktraceBtns = () => wrapper.findAllByTestId('backtrace-expand-btn');
   const findExpandedBacktraceBtnAtIndex = (index) => findExpandBacktraceBtns().at(index);
   const findDetailsLabel = () => wrapper.findByTestId('performance-bar-details-label');
   const findSortOrderSwitcher = () => wrapper.findByTestId('performance-bar-sort-order');
+  const findEmptyDetailNotice = () => wrapper.findByTestId('performance-bar-empty-detail-notice');
   const findAllDetailDurations = () =>
-    wrapper.findAll('[data-testid="performance-item-duration"]').wrappers.map((w) => w.text());
+    wrapper.findAllByTestId('performance-item-duration').wrappers.map((w) => w.text());
   const findAllSummaryItems = () =>
-    wrapper.findAll('[data-testid="performance-bar-summary-item"]').wrappers.map((w) => w.text());
+    wrapper.findAllByTestId('performance-bar-summary-item').wrappers.map((w) => w.text());
 
   afterEach(() => {
     wrapper.destroy();
@@ -38,13 +42,7 @@ describe('detailedMetric', () => {
 
   describe('when the current request has no details', () => {
     beforeEach(() => {
-      createComponent({
-        currentRequest: {},
-        metric: 'gitaly',
-        header: 'Gitaly calls',
-        details: 'details',
-        keys: ['feature', 'request'],
-      });
+      createComponent();
     });
 
     it('does not render the element', () => {
@@ -76,18 +74,15 @@ describe('detailedMetric', () => {
               },
             },
           },
-          metric: 'gitaly',
-          header: 'Gitaly calls',
-          keys: ['feature', 'request'],
         });
       });
 
       it('displays an empty title', () => {
-        expect(findDetailsLabel().text()).toContain('0');
+        expect(findDetailsLabel().text()).toBe('0');
       });
 
       it('displays an empty modal', () => {
-        expect(wrapper.text().replace(/\s+/g, ' ')).toContain('No gitaly calls for this request');
+        expect(findEmptyDetailNotice().text()).toContain('No gitaly calls for this request');
       });
 
       it('does not display sort by switcher', () => {
@@ -112,9 +107,6 @@ describe('detailedMetric', () => {
               },
             },
           },
-          metric: 'gitaly',
-          header: 'Gitaly calls',
-          keys: ['feature', 'request'],
         });
       });
 
@@ -141,14 +133,11 @@ describe('detailedMetric', () => {
               },
             },
           },
-          metric: 'gitaly',
-          header: 'Gitaly calls',
-          keys: ['feature', 'request'],
         });
       });
 
       it('displays details header', () => {
-        expect(findDetailsLabel().text()).toContain('123ms / 456');
+        expect(findDetailsLabel().text()).toBe('123ms / 456');
       });
 
       it('displays a basic summary section', () => {
@@ -156,11 +145,7 @@ describe('detailedMetric', () => {
       });
 
       it('sorts the details by descending duration order', () => {
-        expect(
-          wrapper
-            .findAll('[data-testid="performance-item-duration"]')
-            .wrappers.map((w) => w.text()),
-        ).toEqual(['100ms', '23ms']);
+        expect(findAllDetailDurations()).toEqual(['100ms', '23ms']);
       });
 
       it('does not display sort by switcher', () => {
@@ -275,10 +260,10 @@ describe('detailedMetric', () => {
       });
 
       it('allows switch sorting orders', async () => {
-        findSortOrderSwitcher().vm.$emit('input', SortOrderChronological);
+        findSortOrderSwitcher().vm.$emit('input', sortOrders.CHRONOLOGICAL);
         await nextTick();
         expect(findAllDetailDurations()).toEqual(['23ms', '100ms', '75ms']);
-        findSortOrderSwitcher().vm.$emit('input', SortOrderDuration);
+        findSortOrderSwitcher().vm.$emit('input', sortOrders.DURATION);
         await nextTick();
         expect(findAllDetailDurations()).toEqual(['100ms', '75ms', '23ms']);
       });
@@ -296,10 +281,7 @@ describe('detailedMetric', () => {
               },
             },
           },
-          metric: 'gitaly',
           title: 'custom',
-          header: 'Gitaly calls',
-          keys: ['feature', 'request'],
         });
       });
 
@@ -326,7 +308,7 @@ describe('detailedMetric', () => {
       });
 
       it('displays calls in the label', () => {
-        expect(findDetailsLabel().text()).toContain('456');
+        expect(findDetailsLabel().text()).toBe('456');
       });
 
       it('displays a basic summary section', () => {