-
Notifications
You must be signed in to change notification settings - Fork 345
feat(metadata-view): default sortBy #4232
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.
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
Conversation
WalkthroughAdds item-prefixed metadata field constants and several new permission constants in Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant ContentExplorer
participant MDQuery
User->>ContentExplorer: Open Metadata View (request data)
ContentExplorer->>ContentExplorer: Compute sortBy, check metadataViewV2 flag
alt metadataViewV2 && sortBy == FIELD_NAME
ContentExplorer->>MDQuery: order_by[0].field_key = FIELD_ITEM_NAME, direction = sortDirection
else
ContentExplorer->>MDQuery: order_by[0].field_key = sortBy, direction = sortDirection
end
MDQuery-->>ContentExplorer: Return ordered results
ContentExplorer-->>User: Render results
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. 📜 Recent review detailsConfiguration used: CodeRabbit UI 💡 Knowledge Base configuration:
You can enable these sources in your CodeRabbit configuration. 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
⏰ 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)
✨ 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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/elements/content-explorer/ContentExplorer.tsx (1)
455-461: Gate name mapping behind MDV2 flag & preserve caller-providedorder_by(optional refactor)We always overwrite
metadataQueryClone.order_byand unconditionally mapFIELD_NAME→FIELD_ITEM_NAME, despite the inline note “for metadata view v2 only.” Also, no normalization oforder_by.directionwas found in our helpers—verify thatsortDirectionconstants ('asc'/'desc') match the API’s expected casing, and add a.toLowerCase()here if they don’t.• File:
src/elements/content-explorer/ContentExplorer.tsx(≈lines 455–461)
• Introduce feature flag check and fallback to existingorder_by:- metadataQueryClone.order_by = [ - { - // Default to the prefixed name field for metadata view v2 only, while not touching the default sortBy for other views. - field_key: sortBy === FIELD_NAME ? FIELD_ITEM_NAME : sortBy, - direction: sortDirection, - }, - ]; + const isV2 = isFeatureEnabled(features, 'contentExplorer.metadataViewV2'); + metadataQueryClone.order_by = metadataQueryClone.order_by ?? [ + { + // Default to the prefixed name field for metadata view v2 only, while not touching the default sortBy for other views. + field_key: sortBy === FIELD_NAME && isV2 ? FIELD_ITEM_NAME : sortBy, + direction: sortDirection, // ensure this matches API casing + }, + ];src/elements/content-explorer/stories/tests/MetadataView-visual.stories.tsx (1)
97-109: Await the click to avoid flakiness in the play function.userEvent.click is async in modern @testing-library/user-event. Awaiting it improves reliability.
- userEvent.click(industryHeader); + await userEvent.click(industryHeader);Optional: you can simplify the wait by using findByRole instead of waitFor + getByRole:
- Replace the waitFor block with:
const firstRow = await canvas.findByRole('row', { name: /Industry/i });Do you want me to update the story to use findByRole and add a follow-up assertion once handleSortChange is implemented?
📜 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/constants.js(1 hunks)src/elements/content-explorer/ContentExplorer.tsx(2 hunks)src/elements/content-explorer/stories/tests/MetadataView-visual.stories.tsx(3 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (2)
src/elements/content-explorer/ContentExplorer.tsx (1)
src/constants.js (2)
FIELD_ITEM_NAME(174-174)FIELD_ITEM_NAME(174-174)
src/elements/content-explorer/stories/tests/MetadataView-visual.stories.tsx (1)
src/elements/common/__mocks__/mockMetadata.ts (1)
mockSchema(226-226)
⏰ 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/elements/content-explorer/ContentExplorer.tsx (1)
51-51: Import of FIELD_ITEM_NAME is appropriate for MDV2 sort mapping.This aligns with the new item-prefixed field constants and is used correctly below.
src/elements/content-explorer/stories/tests/MetadataView-visual.stories.tsx (2)
24-35: Switch to order_by looks correct and aligns with MD Query API.Using order_by with field_key/direction is the correct structure for the metadata query. Note ContentExplorer currently overrides order_by by default; if you intend the story-provided order_by to take effect initially, see the optional change suggested in ContentExplorer.
52-64: allowSorting fully replaced with allowsSorting
No occurrences of the oldallowSortingproperty were found in.tsor.tsxfiles—everything has been updated toallowsSorting. All checks pass; no further action needed.src/constants.js (2)
171-183: Item-prefixed field constants are well-scoped and useful for MD Query v2.Non-exported ITEM_PREFIX keeps the surface clean. Names follow existing conventions.
185-197: Approve addition of new permission constantsEach of the newly introduced
PERMISSION_CAN_*constants is defined exactly once insrc/constants.jsand no duplicate exports were found.
change for mdv2 only
Summary by CodeRabbit
New Features
Bug Fixes
Tests