fix(content-sidebar): Clean up taxonomy fetchers #4234
Conversation
WalkthroughRenames Flow fields Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Client
participant Fetcher
participant API
Note over Fetcher,API #E8F8FF: Mapping uses only `displayName`
Client->>Fetcher: Request taxonomy data
Fetcher->>API: Fetch taxonomy payload
API-->>Fetcher: Returns options with `displayName`
alt `displayName` present
Fetcher->>Fetcher: Map item.displayName -> levelName / levelValue / ancestors
else missing `displayName`
Fetcher->>Fetcher: Fields may be undefined (no fallback)
end
Fetcher-->>Client: Return shaped response (includes totalResultCount & limit when provided)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 Biome (2.1.2)src/common/types/metadata.jsbiome: error while loading shared libraries: libpthread.so.0: cannot open shared object file: No such file or directory src/elements/content-sidebar/__tests__/metadataTaxonomyFetcher.test.tsbiome: error while loading shared libraries: libpthread.so.0: cannot open shared object file: No such file or directory src/elements/content-sidebar/fetchers/metadataTaxonomyFetcher.tsbiome: error while loading shared libraries: libpthread.so.0: cannot open shared object file: No such file or directory ✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
src/elements/content-sidebar/__tests__/metadataTaxonomyFetcher.test.ts (2)
22-45: Add an assertion to ensure legacy display_name does not leak through ancestorsGiven the fetcher’s new contract to use displayName only, it’s worth asserting that any legacy display_name fields are not present in the returned ancestors. This protects against regressions.
You can augment the first test with:
// after computing result expect(result.options[1].ancestors?.[0]).not.toHaveProperty('display_name');
94-94: Rename test for clarity; it uses new namingThe test title says "old naming convention" but the data uses displayName. Consider renaming for clarity.
- test('should fetch metadata options and return formatted data - old naming convention', async () => { + test('should fetch metadata options and return formatted data', async () => {src/elements/content-sidebar/fetchers/metadataTaxonomyFetcher.ts (2)
19-33: Pagination marker likely should come from response.next_marker, not request optionsCurrent code echoes the input marker back in the response. Typically, fetchers return the server’s next_marker so the caller can request the next page. Given MetadataOptions has next_marker (per Flow types), consider using it.
If this is the intended contract, ignore. Otherwise, here’s a proposed change:
- const { marker = null } = options; + // Prefer the server-provided next marker; fall back to the request marker if needed + const marker = metadataOptions.next_marker ?? options.marker ?? null; @@ 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, })), marker,If we switch to next_marker, the tests expecting marker === options.marker will need to be updated accordingly. Let me know if you want me to adjust the tests.
60-69: Optional: add Map generics for clarityTyping the map improves readability and avoids any accidental type drift.
Example:
const levelsMap = new Map<number, Partial<HydratedMetadataTaxonomyLevel>>();
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
src/common/types/metadata.js(1 hunks)src/elements/content-sidebar/__tests__/metadataTaxonomyFetcher.test.ts(7 hunks)src/elements/content-sidebar/fetchers/metadataTaxonomyFetcher.ts(4 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
src/elements/content-sidebar/fetchers/metadataTaxonomyFetcher.ts (1)
src/api/Metadata.js (1)
metadataTaxonomyNode(1383-1383)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: lint_test_build
- GitHub Check: Summary
🔇 Additional comments (5)
src/common/types/metadata.js (2)
141-143: Rename to displayName aligns with new API — looks goodThe field rename on MetadataOptionEntryAncestor is consistent with the PR objective and related fetcher/test updates.
147-152: Alignleveltype with actual usage and API contract
Tests formetadataTaxonomyFetchermocklevelas strings ('1','2'), while story fixtures and other metadata APIs treat it as numbers. To prevent type drift:• In
src/common/types/metadata.js, update both declarations// from level: number, // to level: string | number,in
MetadataOptionEntryandMetadataOptionEntryAncestor.
• Optionally, normalize the value inmetadataTaxonomyFetcher(e.g.level: Number(metadataOption.level)) if the API always returns numbers.
• Confirm the backend/API response shape forleveland choose a single representation or keep the union type.src/elements/content-sidebar/__tests__/metadataTaxonomyFetcher.test.ts (1)
311-351: LGTM: ancestors hydration expectations match the new namingThe test aligns with the updated fetcher logic: levelName from taxonomy.levels[i].displayName and levelValue from node/ancestor.displayName. Good coverage of the happy path.
src/elements/content-sidebar/fetchers/metadataTaxonomyFetcher.ts (2)
62-68: LGTM: levelName now sourced from displayNameThis matches the new API naming and the updated tests. No fallback to display_name remains here.
82-88: LGTM: ancestor hydration uses displayNameHydration updates levelValue and id from ancestors using displayName only, consistent with the new API.
| 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, | ||
| })), |
There was a problem hiding this comment.
🛠️ 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.
| 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.
greg-in-a-box
changed the title
DanilaRubleuski
force-pushed
the
WEBAPP-33012
branch
from
40e0b66 to
8dca155
Compare
DanilaRubleuski
force-pushed
the
WEBAPP-33012
branch
from
8dca155 to
cf44f80
Compare
DanilaRubleuski
force-pushed
the
WEBAPP-33012
branch
from
8e33f02 to
bd338a2
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/elements/content-sidebar/__tests__/metadataTaxonomyFetcher.test.ts (1)
15-20: Fix API mock typing or cast it right, fool!
apiMock: jest.Mocked<API>is only partially implemented here; without a cast this will blow up TypeScript. Cast or narrow the type so the suite compiles.Apply this diff to both beforeEach blocks:
- apiMock = { + apiMock = { getMetadataAPI: jest.fn().mockReturnValue({ - getMetadataOptions: jest.fn(), + getMetadataOptions: jest.fn(), }), - }; + } as unknown as jest.Mocked<API>;- apiMock = { + apiMock = { getMetadataAPI: jest.fn().mockReturnValue({ getMetadataTaxonomy: jest.fn(), getMetadataTaxonomyNode: jest.fn(), }), - }; + } as unknown as jest.Mocked<API>;Also applies to: 303-309
🧹 Nitpick comments (4)
src/elements/content-sidebar/__tests__/metadataTaxonomyFetcher.test.ts (4)
94-95: Say what you mean in the test title, fool.This test validates legacy pagination fields (snake_case), not old option-name keys. Rename for clarity.
-test('should fetch metadata options and return formatted data - old naming convention', async () => { +test('should map legacy snake_case pagination fields to camelCase (limit, total_result_count)', async () => {
199-214: Don’t assert undefined fields; verify absence, fool.Expecting
{ ancestors: undefined, nodePath: undefined, ... }is brittle. Prefer matching required fields and asserting missing props explicitly.- expect(result).toEqual({ + expect(result).toMatchObject({ options: [ { value: 'opt1', displayValue: 'Option 1', level: '1', parentId: 'parent1', - nodePath: undefined, - deprecated: undefined, - ancestors: undefined, - selectable: undefined, }, ], marker: 'marker_1', - }); + }); + expect(result.options[0]).not.toHaveProperty('nodePath'); + expect(result.options[0]).not.toHaveProperty('deprecated'); + expect(result.options[0]).not.toHaveProperty('ancestors'); + expect(result.options[0]).not.toHaveProperty('selectable');If the fetcher intentionally sets
undefinedkeys, keep as-is. Otherwise, apply the refactor for stability.
295-299: Keep IDs consistent — use fileId everywhere, fool.
fileIDvsfileIdis noisy. Align naming for readability.-const fileID = '12345'; +const fileId = '12345'; ... - expect(apiMock.getMetadataAPI(false).getMetadataTaxonomy).toHaveBeenCalledWith(fileID, scope, taxonomyKey); + expect(apiMock.getMetadataAPI(false).getMetadataTaxonomy).toHaveBeenCalledWith(fileId, scope, taxonomyKey); ... - const result = await metadataTaxonomyNodeAncestorsFetcher(apiMock, fileID, scope, taxonomyKey, nodeID); + const result = await metadataTaxonomyNodeAncestorsFetcher(apiMock, fileId, scope, taxonomyKey, nodeID); ... - await expect(metadataTaxonomyNodeAncestorsFetcher(apiMock, fileID, scope, taxonomyKey, nodeID)).rejects.toThrow( + await expect(metadataTaxonomyNodeAncestorsFetcher(apiMock, fileId, scope, taxonomyKey, nodeID)).rejects.toThrow(Also applies to: 354-362, 384-396, 403-416
365-372: Add a null-ancestors case too, sucka.You cover empty array; add a test where
ancestors: nullto ensure identical behavior.Example to add nearby:
test('should handle null ancestors as empty', async () => { const mockTaxonomy = { displayName: 'Geography', namespace: 'my_enterprise', id: 'my_id', key: 'geography', levels: [{ level: 1, displayName: 'Level 1', description: 'Description 1' }] }; const mockTaxonomyNode = { id: 'node_abc', level: 1, displayName: 'Node ABC', ancestors: null as any }; apiMock.getMetadataAPI(false).getMetadataTaxonomy.mockResolvedValue(mockTaxonomy); apiMock.getMetadataAPI(false).getMetadataTaxonomyNode.mockResolvedValue(mockTaxonomyNode); const result = await metadataTaxonomyNodeAncestorsFetcher(apiMock, fileId, scope, taxonomyKey, nodeID); expect(result).toEqual([{ level: 1, levelName: 'Level 1', description: 'Description 1', id: 'node_abc', levelValue: 'Node ABC' }]); });Also applies to: 377-379
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/common/types/metadata.js(1 hunks)src/elements/content-sidebar/__tests__/metadataTaxonomyFetcher.test.ts(7 hunks)src/elements/content-sidebar/fetchers/metadataTaxonomyFetcher.ts(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/common/types/metadata.js
- src/elements/content-sidebar/fetchers/metadataTaxonomyFetcher.ts
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: lint_test_build
- GitHub Check: Summary
🔇 Additional comments (4)
src/elements/content-sidebar/__tests__/metadataTaxonomyFetcher.test.ts (4)
24-44: Nice displayName cleanup — no more snake_case, sucka!Switch to
displayNamein entries and nestedancestorslooks consistent and aligns with the new API.
248-279: Backward-compat test looks tight, fool.Null ancestors handled, optional fields omitted, selectable preserved. Solid coverage.
313-321: Ancestors fetcher: displayName usage and level mapping look sharp.Taxonomy levels map to
levelName/description, node/ancestor names tolevelValue. Clean and coherent.Also applies to: 327-329
219-246: Marker passthrough logic is correctI pity the fool who didn’t see that the fetcher destructures
markerfrom the input options (const { marker = null } = options) and returns it unchanged, so the test’smarker: nullexpectation is valid.Likely an incorrect or invalid review comment.
2 participants
Description
As a part of Metadata pubic API changes, after successful launch of a new API with new naming we need to cleanup old
display_namenaming formetadataTaxonomyFetcherSummary by CodeRabbit
Refactor
Tests
Bug Fixes