Skip to content

Commit e4d9b49

Browse files
authored
fix(explore): Insert group bys before visualizes if possible (#97923)
When adding a group by from the toolbar, we want them to appear before the visualizes when the existing aggregate fields are sorted with group bys first then visualizes. Closes EXP-380
1 parent e9ad12f commit e4d9b49

File tree

2 files changed

+28
-8
lines changed

2 files changed

+28
-8
lines changed

static/app/views/explore/contexts/pageParamsContext/index.spec.tsx

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -157,10 +157,10 @@ describe('PageParamsProvider', () => {
157157
aggregateSortBys: [{field: 'count(span.self_time)', kind: 'asc'}],
158158
aggregateFields: [
159159
{groupBy: 'browser.name'},
160+
{groupBy: 'sdk.name'},
160161
new Visualize('count(span.self_time)', {
161162
chartType: ChartType.AREA,
162163
}),
163-
{groupBy: 'sdk.name'},
164164
],
165165
})
166166
);

static/app/views/explore/contexts/pageParamsContext/index.tsx

Lines changed: 27 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -577,20 +577,40 @@ export function useSetExploreGroupBys() {
577577
return useCallback(
578578
(groupBys: string[], mode?: Mode) => {
579579
const aggregateFields = [];
580-
let i = 0;
580+
581+
let seenVisualizes = false;
582+
let groupByAfterVisualizes = false;
583+
584+
for (const aggregateField of pageParams.aggregateFields) {
585+
if (isGroupBy(aggregateField) && seenVisualizes) {
586+
groupByAfterVisualizes = true;
587+
break;
588+
} else if (isVisualize(aggregateField)) {
589+
seenVisualizes = true;
590+
}
591+
}
592+
593+
const iter = groupBys[Symbol.iterator]();
594+
581595
for (const aggregateField of pageParams.aggregateFields) {
582596
if (isGroupBy(aggregateField)) {
583-
if (i < groupBys.length) {
584-
const groupBy: GroupBy = {groupBy: groupBys[i++]!};
585-
aggregateFields.push(groupBy);
597+
const {value: groupBy, done} = iter.next();
598+
if (!done) {
599+
aggregateFields.push({groupBy});
586600
}
587601
} else {
602+
if (!groupByAfterVisualizes) {
603+
// no existing group by appears after a visualize, so any additional
604+
// group bys will be inserted before any visualizes as well
605+
for (const groupBy of iter) {
606+
aggregateFields.push({groupBy});
607+
}
608+
}
588609
aggregateFields.push(aggregateField.toJSON());
589610
}
590611
}
591-
for (; i < groupBys.length; i++) {
592-
const groupBy = {groupBy: groupBys[i]!};
593-
aggregateFields.push(groupBy);
612+
for (const groupBy of iter) {
613+
aggregateFields.push({groupBy});
594614
}
595615

596616
setPageParams({aggregateFields, mode});

0 commit comments

Comments
 (0)