-
Notifications
You must be signed in to change notification settings - Fork 40
feat(recent/favorite): apply new recent/favorite feature #6137
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
feat(recent/favorite): apply new recent/favorite feature #6137
Conversation
Signed-off-by: samuel.park <[email protected]>
Signed-off-by: samuel.park <[email protected]>
Signed-off-by: samuel.park <[email protected]>
Signed-off-by: samuel.park <[email protected]>
Signed-off-by: samuel.park <[email protected]>
Signed-off-by: samuel.park <[email protected]>
Signed-off-by: samuel.park <[email protected]>
Signed-off-by: samuel.park <[email protected]>
Signed-off-by: samuel.park <[email protected]>
The latest updates on your projects. Learn more about Vercel for GitHub. 3 Skipped Deployments
|
🎉 @yuda110 has been randomly selected as the reviewer! Please review. 🙏 |
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.
Pull Request Overview
This pull request implements a new recent/favorite feature by migrating from the old user-config structure to a new approach using composables and mutations. The changes standardize how recent items and favorites are managed across the application.
Key changes include:
- Replacement of store-based recent and favorite management with composable-based approach
- Migration from
useRecentStore
touseRecentDelete
anduseFavoriteStore
touseFavoriteList
anduseFavoriteDeleteMutation
- Updates to router recent helper to use async operations with query client invalidation
- Type updates changing
FavoriteItem
toFavoriteConfig
for consistency
Reviewed Changes
Copilot reviewed 29 out of 29 changed files in this pull request and generated 8 comments.
Show a summary per file
File | Description |
---|---|
UserConfigRecent.vue | Fixed bug where recentConfig.id should be recentConfig.itemId for recent item lookups |
ServiceAccountDeleteModal.vue | Migrated from useRecentStore to useRecentDelete composable |
Multiple project files | Replaced useFavoriteStore with useFavoriteList and useFavoriteDeleteMutation |
router/index.ts | Updated router to use async recent config setting with query client |
router-recent-helper.ts | Refactored from synchronous to asynchronous recent config operations |
Navigation and search modules | Updated to use new user config approach and removed deprecated metric explorer functionality |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
@@ -73,18 +73,18 @@ const state = reactive({ | |||
const convertRecentToReferenceData = (recentConfig: ConfigData): ReferenceData|undefined => { | |||
const { itemType } = recentConfig; | |||
if (itemType === RECENT_TYPE.DASHBOARD) { | |||
return convertedRecentConfigData.convertedDashboard.value.find((d) => d.itemId === recentConfig.id); | |||
return convertedRecentConfigData.convertedDashboard.value.find((d) => d.itemId === recentConfig.itemId); |
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.
The change from recentConfig.id
to recentConfig.itemId
is correct and fixes a property access bug. The ConfigData
interface uses itemId
as the property name, not id
.
Copilot uses AI. Check for mistakes.
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.
LGTM!!
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.
Cleeeeeeaaaaaaan
@@ -143,7 +140,7 @@ export const useTopBarSearchStore = defineStore('top-bar-search', () => { | |||
}; | |||
watch([() => getters.trimmedInputText, () => state.activeTab], (trimmedText) => { | |||
state.loading = true; | |||
state.recentMenuList = []; | |||
// state.recentMenuList = []; |
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.
Question: Why annotated all code related of recentMenuList?
Skip Review (optional)
style
,chore
,ci
,test
,docs
)Description (optional)
SSIA
Things to Talk About (optional)