Skip to content

fix(content-sidebar): Clean up taxonomy fetchers #4234

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

fix(content-sidebar): Clean up taxonomy fetchers #4234

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions src/common/types/metadata.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -138,13 +138,13 @@ type MetadataSuggestion = {

type MetadataOptionEntryAncestor = {
id: string,
display_name: string,
displayName: string,
level: string,
};

type MetadataOptionEntry = {
id: string,
display_name: string,
displayName: string,
level: number,
ancestors: MetadataOptionEntryAncestor[],
deprecated: boolean,
Expand Down
137 changes: 15 additions & 122 deletions src/elements/content-sidebar/__tests__/metadataTaxonomyFetcher.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ describe('metadataTaxonomyFetcher', () => {
entries: [
{
id: 'opt1',
display_name: 'Option 1',
displayName: 'Option 1',
level: '1',
parentId: 'parent1',
nodePath: ['node1', 'node2'],
Expand All @@ -34,12 +34,12 @@ describe('metadataTaxonomyFetcher', () => {
},
{
id: 'opt2',
display_name: 'Option 2',
displayName: 'Option 2',
level: '2',
parentId: 'parent2',
nodePath: ['node1', 'node3'],
deprecated: true,
ancestors: [{ display_name: 'Option 1', foo: 'bar' }],
ancestors: [{ displayName: 'Option 1', foo: 'bar' }],
selectable: true
},
],
Expand Down Expand Up @@ -185,7 +185,7 @@ describe('metadataTaxonomyFetcher', () => {
entries: [
{
id: 'opt1',
display_name: 'Option 1',
displayName: 'Option 1',
level: '1',
parentId: 'parent1',
},
Expand Down Expand Up @@ -220,7 +220,7 @@ describe('metadataTaxonomyFetcher', () => {
const mockMetadataOptions = {
entries: [{
id: 'opt1',
display_name: 'Option 1',
displayName: 'Option 1',
parentId: undefined,
nodePath: undefined,
deprecated: undefined,
Expand Down Expand Up @@ -250,7 +250,7 @@ describe('metadataTaxonomyFetcher', () => {
entries: [
{
id: 'opt1',
display_name: 'Option 1',
displayName: 'Option 1',
level: '1',
ancestors: null,
selectable: false
Expand Down Expand Up @@ -291,113 +291,6 @@ describe('metadataTaxonomyFetcher', () => {
});
});

// TODO: delete whole section during clean up as for now we have to handle both new and old naming convention
describe('metadataTaxonomyNodeAncestorsFetcher (old keys naming convention)', () => {
const fileID = '12345';
const scope = 'global';
const taxonomyKey = 'taxonomy_123';
const nodeID = 'node_abc';

let apiMock: jest.Mocked<API>;

beforeEach(() => {
apiMock = {
getMetadataAPI: jest.fn().mockReturnValue({
getMetadataTaxonomy: jest.fn(),
getMetadataTaxonomyNode: jest.fn(),
}),
};
});

test('should fetch taxonomy and node data and return formatted data', async () => {
const mockTaxonomy = {
display_name: 'Geography',
namespace: 'my_enterprise',
id: 'my_id',
key: 'geography',
levels: [
{ level: 1, display_name: 'Level 1', description: 'Description 1' },
{ level: 2, display_name: 'Level 2', description: 'Description 2' },
{ level: 3, display_name: 'Level 3', description: 'Description 3' },
],
};

const mockTaxonomyNode = {
id: 'node_abc',
level: 1,
display_name: 'Node ABC',
ancestors: [{ id: 'ancestor_1', level: 2, display_name: 'Ancestor 1' }],
};

apiMock.getMetadataAPI(false).getMetadataTaxonomy.mockResolvedValue(mockTaxonomy);
apiMock.getMetadataAPI(false).getMetadataTaxonomyNode.mockResolvedValue(mockTaxonomyNode);

const result = await metadataTaxonomyNodeAncestorsFetcher(apiMock, fileID, scope, taxonomyKey, nodeID);

const expectedResult = [
{
level: 1,
levelName: 'Level 1',
description: 'Description 1',
id: 'node_abc',
levelValue: 'Node ABC',
},
{
level: 2,
levelName: 'Level 2',
description: 'Description 2',
id: 'ancestor_1',
levelValue: 'Ancestor 1',
},
];

expect(apiMock.getMetadataAPI).toHaveBeenCalledWith(false);
expect(apiMock.getMetadataAPI(false).getMetadataTaxonomy).toHaveBeenCalledWith(fileID, scope, taxonomyKey);
expect(apiMock.getMetadataAPI(false).getMetadataTaxonomyNode).toHaveBeenCalledWith(
fileID,
scope,
taxonomyKey,
nodeID,
true,
);
expect(result).toEqual(expectedResult);
});

test('should handle empty ancestors array', async () => {
const mockTaxonomy = {
display_name: 'Geography',
namespace: 'my_enterprise',
id: 'my_id',
key: 'geography',
levels: [{ level: 1, display_name: 'Level 1', description: 'Description 1' }],
};

const mockTaxonomyNode = {
id: 'node_abc',
level: 1,
display_name: 'Node ABC',
ancestors: [],
};

apiMock.getMetadataAPI(false).getMetadataTaxonomy.mockResolvedValue(mockTaxonomy);
apiMock.getMetadataAPI(false).getMetadataTaxonomyNode.mockResolvedValue(mockTaxonomyNode);

const result = await metadataTaxonomyNodeAncestorsFetcher(apiMock, fileID, scope, taxonomyKey, nodeID);

const expectedResult = [
{
level: 1,
levelName: 'Level 1',
description: 'Description 1',
id: 'node_abc',
levelValue: 'Node ABC',
},
];

expect(result).toEqual(expectedResult);
});
});

describe('metadataTaxonomyNodeAncestorsFetcher (new keys naming convention)', () => {
const fileID = '12345';
const scope = 'global';
Expand All @@ -417,22 +310,22 @@ describe('metadataTaxonomyNodeAncestorsFetcher (new keys naming convention)', ()

test('should fetch taxonomy and node data and return formatted data', async () => {
const mockTaxonomy = {
display_name: 'Geography',
displayName: 'Geography',
namespace: 'my_enterprise',
id: 'my_id',
key: 'geography',
levels: [
{ level: 1, display_name: 'Level 1', description: 'Description 1' },
{ level: 2, display_name: 'Level 2', description: 'Description 2' },
{ level: 3, display_name: 'Level 3', description: 'Description 3' },
{ level: 1, displayName: 'Level 1', description: 'Description 1' },
{ level: 2, displayName: 'Level 2', description: 'Description 2' },
{ level: 3, displayName: 'Level 3', description: 'Description 3' },
],
};

const mockTaxonomyNode = {
id: 'node_abc',
level: 1,
display_name: 'Node ABC',
ancestors: [{ id: 'ancestor_1', level: 2, display_name: 'Ancestor 1' }],
displayName: 'Node ABC',
ancestors: [{ id: 'ancestor_1', level: 2, displayName: 'Ancestor 1' }],
};

apiMock.getMetadataAPI(false).getMetadataTaxonomy.mockResolvedValue(mockTaxonomy);
Expand Down Expand Up @@ -471,17 +364,17 @@ describe('metadataTaxonomyNodeAncestorsFetcher (new keys naming convention)', ()

test('should handle empty ancestors array', async () => {
const mockTaxonomy = {
display_name: 'Geography',
displayName: 'Geography',
namespace: 'my_enterprise',
id: 'my_id',
key: 'geography',
levels: [{ level: 1, display_name: 'Level 1', description: 'Description 1' }],
levels: [{ level: 1, displayName: 'Level 1', description: 'Description 1' }],
};

const mockTaxonomyNode = {
id: 'node_abc',
level: 1,
display_name: 'Node ABC',
displayName: 'Node ABC',
ancestors: [],
};

Expand Down
10 changes: 5 additions & 5 deletions src/elements/content-sidebar/fetchers/metadataTaxonomyFetcher.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,12 +19,12 @@ export const metadataTaxonomyFetcher = async (
return {
options: metadataOptions.entries.map((metadataOption: MetadataOptionEntry) => ({
value: metadataOption.id,
displayValue: metadataOption.display_name || metadataOption.displayName,
displayValue: metadataOption.displayName,
level: metadataOption.level,
parentId: metadataOption.parentId,
nodePath: metadataOption.nodePath,
deprecated: metadataOption.deprecated,
ancestors: metadataOption.ancestors?.map(({display_name, displayName, ...rest}) => ({...rest, displayName: display_name || displayName })),
ancestors: metadataOption.ancestors?.map(({displayName, ...rest}) => ({...rest, displayName })),
selectable: metadataOption.selectable,
})),
Comment on lines 20 to 29
Copy link
Contributor

@coderabbitai coderabbitai bot Aug 20, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Strip legacy display_name from ancestors to avoid leaking snake_case fields

The current mapping preserves all other fields via ...rest. If the API still sends display_name on ancestors, that key will pass through to consumers. Since this PR’s goal is to remove legacy display_name usage, we should explicitly omit it.

Apply this diff:

-            ancestors: metadataOption.ancestors?.map(({displayName, ...rest}) => ({...rest, displayName })),
+            ancestors: metadataOption.ancestors?.map(({ displayName, display_name, ...rest }) => ({
+                ...rest,
+                displayName,
+            })),
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
options: metadataOptions.entries.map((metadataOption: MetadataOptionEntry) => ({
value: metadataOption.id,
displayValue: metadataOption.display_name || metadataOption.displayName,
displayValue: metadataOption.displayName,
level: metadataOption.level,
parentId: metadataOption.parentId,
nodePath: metadataOption.nodePath,
deprecated: metadataOption.deprecated,
ancestors: metadataOption.ancestors?.map(({display_name, displayName, ...rest}) => ({...rest, displayName: display_name || displayName })),
ancestors: metadataOption.ancestors?.map(({displayName, ...rest}) => ({...rest, displayName })),
selectable: metadataOption.selectable,
})),
options: metadataOptions.entries.map((metadataOption: MetadataOptionEntry) => ({
value: metadataOption.id,
displayValue: metadataOption.displayName,
level: metadataOption.level,
parentId: metadataOption.parentId,
nodePath: metadataOption.nodePath,
deprecated: metadataOption.deprecated,
ancestors: metadataOption.ancestors?.map(({ displayName, display_name, ...rest }) => ({
...rest,
displayName,
})),
selectable: metadataOption.selectable,
})),
🤖 Prompt for AI Agents 
In src/elements/content-sidebar/fetchers/metadataTaxonomyFetcher.ts around lines
20 to 29, the ancestors mapping currently preserves any legacy snake_case
display_name field via ...rest; update the mapping to explicitly destructure and
omit display_name (and keep displayName) so the returned ancestor objects
include displayName but do not include legacy display_name — e.g., destructure
ancestors items to pull out displayName and display_name (or explicitly remove
display_name) and then return {...rest, displayName} so legacy keys are not
passed through to consumers.

marker,
Expand Down Expand Up @@ -62,7 +62,7 @@ export const metadataTaxonomyNodeAncestorsFetcher = async (
for (const item of metadataTaxonomy.levels) {
const levelData = {
level: item.level,
levelName: item.displayName || item.display_name,
levelName: item.displayName,
description: item.description,
};

Expand All @@ -71,7 +71,7 @@ export const metadataTaxonomyNodeAncestorsFetcher = async (
levelsMap.set(item.level, {
...levelData,
id: metadataTaxonomyNode.id,
levelValue: metadataTaxonomyNode.displayName || metadataTaxonomyNode.display_name,
levelValue: metadataTaxonomyNode.displayName,
});
// If the level is not the metadataTaxonomyNode level, just add the level data
} else {
Expand All @@ -84,7 +84,7 @@ export const metadataTaxonomyNodeAncestorsFetcher = async (
const levelData = levelsMap.get(ancestor.level);

if (levelData) {
levelsMap.set(ancestor.level, { ...levelData, levelValue: ancestor.displayName || ancestor.display_name, id: ancestor.id });
levelsMap.set(ancestor.level, { ...levelData, levelValue: ancestor.displayName, id: ancestor.id });
}
}
}
Expand Down