Skip to content

Commit d55ae51

Browse files
[Discover] Tab duplication naming refactor (#230538)
## Summary Closes #223899 This PR unifies and simplifies getting next tab number and next copy number. I moved the function to `kbn-unified-tabs` package, so Discover is only consuming it, instead of introducing its own logic. ### Checklist Check the PR satisfies following conditions. Reviewers should verify this PR satisfies this list as well. - [ ] Any text added follows [EUI's writing guidelines](https://elastic.github.io/eui/#/guidelines/writing), uses sentence case text and includes [i18n support](https://github.com/elastic/kibana/blob/main/src/platform/packages/shared/kbn-i18n/README.md) - [ ] [Documentation](https://www.elastic.co/guide/en/kibana/master/development-documentation.html) was added for features that require explanation or tutorials - [x] [Unit or functional tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html) were updated or added to match the most common scenarios - [ ] If a plugin configuration key changed, check if it needs to be allowlisted in the cloud and added to the [docker list](https://github.com/elastic/kibana/blob/main/src/dev/build/tasks/os_packages/docker_generator/resources/base/bin/kibana-docker) - [ ] This was checked for breaking HTTP API changes, and any breaking changes have been approved by the breaking-change committee. The `release_note:breaking` label should be applied in these situations. - [ ] [Flaky Test Runner](https://ci-stats.kibana.dev/trigger_flaky_test_runner/1) was used on any tests changed - [x] The PR description includes the appropriate Release Notes section, and the correct `release_note:*` label is applied per the [guidelines](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process) - [x] Review the [backport guidelines](https://docs.google.com/document/d/1VyN5k91e5OVumlc0Gb9RPa3h1ewuPE705nRtioPiTvY/edit?usp=sharing) and apply applicable `backport:*` labels. ### Identify risks Does this PR introduce any risks? For example, consider risks like hard to test bugs, performance regression, potential of data loss. Describe the risk, its severity, and mitigation for each identified risk. Invite stakeholders and evaluate how to proceed before merging. - [ ] [See some risk examples](https://github.com/elastic/kibana/blob/main/RISK_MATRIX.mdx) - [ ] ... --------- Co-authored-by: kibanamachine <[email protected]>
1 parent 14828f6 commit d55ae51

File tree

5 files changed

+190
-36
lines changed

5 files changed

+190
-36
lines changed

src/platform/packages/shared/kbn-unified-tabs/index.ts

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -14,3 +14,4 @@ export {
1414
type TabbedContentProps as UnifiedTabsProps,
1515
} from './src/components/tabbed_content';
1616
export { useNewTabProps } from './src/hooks/use_new_tab_props';
17+
export { getNextTabNumber } from './src/utils/get_next_tab_number';

src/platform/packages/shared/kbn-unified-tabs/src/components/tabbed_content/tabbed_content.tsx

Lines changed: 11 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -25,6 +25,7 @@ import {
2525
closeTabsToTheRight,
2626
} from '../../utils/manage_tabs';
2727
import type { TabItem, TabsServices, TabPreviewData } from '../../types';
28+
import { getNextTabNumber } from '../../utils/get_next_tab_number';
2829

2930
export interface TabbedContentProps extends Pick<TabsBarProps, 'maxItemsCount'> {
3031
items: TabItem[];
@@ -136,27 +137,16 @@ export const TabbedContent: React.FC<TabbedContentProps> = ({
136137
newItem.duplicatedFromId = item.id;
137138

138139
const copyLabel = i18n.translate('unifiedTabs.copyLabel', { defaultMessage: 'copy' });
139-
const escapedCopyLabel = escapeRegExp(copyLabel);
140-
const baseRegex = new RegExp(`\\s*\\(${escapedCopyLabel}\\)( \\d+)?$`);
141-
const baseLabel = item.label.replace(baseRegex, '');
142-
const escapedBaseLabel = escapeRegExp(baseLabel);
143-
144-
// Find all existing copies to determine next number
145-
const copyRegex = new RegExp(`^${escapedBaseLabel}\\s*\\(${escapedCopyLabel}\\)( \\d+)?$`);
146-
const copyNumberRegex = new RegExp(`\\(${escapedCopyLabel}\\) (\\d+)$`);
147-
const copies = state.items
148-
.filter((tab) => copyRegex.test(tab.label))
149-
.map((tab) => {
150-
const match = tab.label.match(copyNumberRegex);
151-
return match && match[1] ? Number(match[1]) : 1; // match[1] is the number after (copy)
152-
});
153-
154-
// Determine the next copy number
155-
const nextNumber = copies.length > 0 ? Math.max(...copies) + 1 : null;
156-
157-
newItem.label = nextNumber
158-
? `${baseLabel} (${copyLabel}) ${nextNumber}`
159-
: `${baseLabel} (${copyLabel})`;
140+
141+
// Remove existing (copy) or (copy N) suffix to get base label
142+
const copyPattern = `\\s*\\(${escapeRegExp(copyLabel)}\\)(?:\\s+\\d+)?$`;
143+
const baseLabel = item.label.replace(new RegExp(copyPattern), '');
144+
145+
// Create the copy base pattern: "Original Label (copy)"
146+
const copyBaseLabel = `${baseLabel} (${copyLabel})`;
147+
148+
const nextNumber = getNextTabNumber(state.items, copyBaseLabel);
149+
newItem.label = nextNumber ? `${copyBaseLabel} ${nextNumber}` : copyBaseLabel;
160150

161151
tabsBarApi.current?.moveFocusToNextSelectedItem(newItem);
162152
changeState((prevState) => insertTabAfter(prevState, newItem, item, maxItemsCount));
Lines changed: 134 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,134 @@
1+
/*
2+
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
3+
* or more contributor license agreements. Licensed under the "Elastic License
4+
* 2.0", the "GNU Affero General Public License v3.0 only", and the "Server Side
5+
* Public License v 1"; you may not use this file except in compliance with, at
6+
* your election, the "Elastic License 2.0", the "GNU Affero General Public
7+
* License v3.0 only", or the "Server Side Public License, v 1".
8+
*/
9+
10+
import { getNextTabNumber } from './get_next_tab_number';
11+
import type { TabItem } from '../types';
12+
13+
const DEFAULT_TAB_LABEL = 'Untitled';
14+
const DEFAULT_TAB_COPY_LABEL = 'Untitled (copy)';
15+
16+
describe('getNextNumber', () => {
17+
it('should return null when no tabs match the base label pattern', () => {
18+
const tabs: TabItem[] = [
19+
{ id: '1', label: 'Some tab' },
20+
{ id: '2', label: 'Another tab' },
21+
{ id: '3', label: 'Different label' },
22+
];
23+
24+
const result = getNextTabNumber(tabs, DEFAULT_TAB_LABEL);
25+
26+
expect(result).toBeNull();
27+
});
28+
29+
it('should work with regular numbered tabs (non-copy scenario)', () => {
30+
const tabs: TabItem[] = [
31+
{ id: '1', label: 'Untitled' },
32+
{ id: '2', label: 'Untitled 2' },
33+
{ id: '3', label: 'Untitled 4' },
34+
{ id: '4', label: 'Some other tab' },
35+
];
36+
37+
const result = getNextTabNumber(tabs, DEFAULT_TAB_LABEL);
38+
39+
expect(result).toBe(5); // Should be max(1, 2, 4) + 1 = 5
40+
});
41+
42+
it('should return 2 when there is one copy tab without a number', () => {
43+
const tabs: TabItem[] = [
44+
{ id: '1', label: 'Untitled' },
45+
{ id: '2', label: 'Untitled (copy)' },
46+
{ id: '3', label: 'Some other tab' },
47+
];
48+
49+
const result = getNextTabNumber(tabs, DEFAULT_TAB_COPY_LABEL);
50+
51+
expect(result).toBe(2); // "Untitled (copy)" is treated as number 1
52+
});
53+
54+
it('should return the next number when multiple numbered copy tabs exist', () => {
55+
const tabs: TabItem[] = [
56+
{ id: '1', label: 'Untitled' },
57+
{ id: '2', label: 'Untitled (copy)' },
58+
{ id: '3', label: 'Untitled (copy) 2' },
59+
{ id: '4', label: 'Untitled (copy) 5' },
60+
{ id: '5', label: 'Some other tab' },
61+
];
62+
63+
const result = getNextTabNumber(tabs, DEFAULT_TAB_COPY_LABEL);
64+
65+
expect(result).toBe(6); // Should be max(1, 2, 5) + 1 = 6
66+
});
67+
68+
it('should count copy and regular tabs separately', () => {
69+
const tabs: TabItem[] = [
70+
{ id: '1', label: 'Untitled 3' },
71+
{ id: '2', label: 'Untitled (copy) 2' },
72+
{ id: '3', label: 'Some other tab' },
73+
];
74+
75+
const nextCopyNumber = getNextTabNumber(tabs, DEFAULT_TAB_COPY_LABEL);
76+
expect(nextCopyNumber).toBe(3);
77+
78+
const nextTabNumber = getNextTabNumber(tabs, DEFAULT_TAB_LABEL);
79+
expect(nextTabNumber).toBe(4);
80+
});
81+
82+
it('should handle tabs with whitespace correctly', () => {
83+
const tabs: TabItem[] = [
84+
{ id: '1', label: ' Untitled ' },
85+
{ id: '2', label: 'Untitled 2 ' },
86+
{ id: '3', label: ' Untitled 3' },
87+
];
88+
89+
const result = getNextTabNumber(tabs, DEFAULT_TAB_LABEL);
90+
91+
expect(result).toBe(4); // Should be max(1, 2, 3) + 1 = 4
92+
});
93+
94+
it('should handle case with only base label', () => {
95+
const tabs: TabItem[] = [{ id: '1', label: 'Untitled' }];
96+
97+
const result = getNextTabNumber(tabs, DEFAULT_TAB_LABEL);
98+
99+
expect(result).toBe(2); // "Untitled" is treated as number 1, so next is 2
100+
});
101+
102+
it('should ignore differently named tabs', () => {
103+
const tabs: TabItem[] = [
104+
{ id: '1', label: 'Untitled' },
105+
{ id: '2', label: 'Untitled abc 4' }, // Invalid - should be ignored
106+
{ id: '3', label: 'Untitled 2' },
107+
{ id: '4', label: 'Untitled extra text 2' }, // Invalid - should be ignored
108+
];
109+
110+
const result = getNextTabNumber(tabs, DEFAULT_TAB_LABEL);
111+
112+
expect(result).toBe(3); // Should be max(1, 2) + 1 = 3
113+
});
114+
115+
it('should work with complex base labels containing special characters', () => {
116+
const tabs: TabItem[] = [
117+
{ id: '1', label: 'My Tab (special/?)' },
118+
{ id: '2', label: 'My Tab (special*) 2' },
119+
{ id: '3', label: 'My Tab (^special) 5' },
120+
];
121+
122+
const result = getNextTabNumber(tabs, 'My Tab (special/?)');
123+
124+
expect(result).toBe(2);
125+
});
126+
127+
it('should handle empty tabs array', () => {
128+
const tabs: TabItem[] = [];
129+
130+
const result = getNextTabNumber(tabs, DEFAULT_TAB_LABEL);
131+
132+
expect(result).toBeNull();
133+
});
134+
});
Lines changed: 40 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,40 @@
1+
/*
2+
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
3+
* or more contributor license agreements. Licensed under the "Elastic License
4+
* 2.0", the "GNU Affero General Public License v3.0 only", and the "Server Side
5+
* Public License v 1"; you may not use this file except in compliance with, at
6+
* your election, the "Elastic License 2.0", the "GNU Affero General Public
7+
* License v3.0 only", or the "Server Side Public License, v 1".
8+
*/
9+
10+
import type { TabItem } from '../types';
11+
12+
export const getNextTabNumber = (allTabs: TabItem[], baseLabel: string) => {
13+
// Find the highest number among tabs with the base label
14+
const maxNumber = allTabs.reduce((max, tab) => {
15+
const label = tab.label.trim();
16+
17+
// Check if this is the base label without a number (implicit "1")
18+
if (label === baseLabel) {
19+
return Math.max(max, 1);
20+
}
21+
22+
// Check if this is a numbered variant like "Base Label 2"
23+
if (label.startsWith(`${baseLabel} `)) {
24+
// Extract the number part after "Base Label "
25+
const suffix = label.slice(baseLabel.length + 1);
26+
const num = Number(suffix);
27+
28+
// Only consider valid numbers
29+
if (!isNaN(num)) {
30+
return Math.max(max, num);
31+
}
32+
}
33+
34+
// Tab doesn't match our pattern, keep current max
35+
return max;
36+
}, 0); // Start with 0 as initial max
37+
38+
// Return next number if we found any matching tabs, otherwise null
39+
return maxNumber > 0 ? maxNumber + 1 : null;
40+
};

src/platform/plugins/shared/discover/public/application/main/state_management/redux/utils.ts

Lines changed: 4 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -8,9 +8,8 @@
88
*/
99

1010
import { v4 as uuid } from 'uuid';
11-
import { escapeRegExp } from 'lodash';
1211
import { i18n } from '@kbn/i18n';
13-
import type { TabItem } from '@kbn/unified-tabs';
12+
import { getNextTabNumber, type TabItem } from '@kbn/unified-tabs';
1413
import { createAsyncThunk } from '@reduxjs/toolkit';
1514
import type { DiscoverInternalState, TabState } from './types';
1615
import type {
@@ -47,23 +46,13 @@ export type TabActionInjector = ReturnType<typeof createTabActionInjector>;
4746
const DEFAULT_TAB_LABEL = i18n.translate('discover.defaultTabLabel', {
4847
defaultMessage: 'Untitled',
4948
});
50-
const ESCAPED_DEFAULT_TAB_LABEL = escapeRegExp(DEFAULT_TAB_LABEL);
51-
const DEFAULT_TAB_REGEX = new RegExp(`^${ESCAPED_DEFAULT_TAB_LABEL}( \\d+)?$`); // any default tab
52-
const DEFAULT_TAB_NUMBER_REGEX = new RegExp(`^${ESCAPED_DEFAULT_TAB_LABEL} (?<tabNumber>\\d+)$`); // tab with a number
5349

5450
export const createTabItem = (allTabs: TabState[]): TabItem => {
5551
const id = uuid();
52+
const baseLabel = DEFAULT_TAB_LABEL;
5653

57-
const existingNumbers = allTabs
58-
.filter((tab) => DEFAULT_TAB_REGEX.test(tab.label.trim()))
59-
.map((tab) => {
60-
const match = tab.label.trim().match(DEFAULT_TAB_NUMBER_REGEX);
61-
const tabNumber = match?.groups?.tabNumber;
62-
return tabNumber ? Number(tabNumber) : 1;
63-
});
64-
65-
const nextNumber = existingNumbers.length > 0 ? Math.max(...existingNumbers) + 1 : null;
66-
const label = nextNumber ? `${DEFAULT_TAB_LABEL} ${nextNumber}` : DEFAULT_TAB_LABEL;
54+
const nextNumber = getNextTabNumber(allTabs, baseLabel);
55+
const label = nextNumber ? `${baseLabel} ${nextNumber}` : baseLabel;
6756

6857
return { id, label };
6958
};

0 commit comments

Comments
 (0)