-
Notifications
You must be signed in to change notification settings - Fork 40
chore(service-pagination): fix bug about queryTag with pagination #5865
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
Signed-off-by: seungyeoneeee <sylee1274@mz.co.kr>
Signed-off-by: seungyeoneeee <sylee1274@mz.co.kr>
Signed-off-by: 이승연 <sylee1274@mz.co.kr>
|
✅ There are no commits in this PR that require review. |
|
🎉 @skdud4659 and @WANZARGEN have been randomly selected as the reviewers! Please review. 🙏 |
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎ 4 Skipped Deployments
|
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 PR fixes a bug related to query tag handling with pagination within the Alert Manager service module.
- Introduces a new state property (searchFilters) and corresponding setter in the service-list-page-store.
- Refactors query tag and pagination logic in ServiceList.vue, including updating how query parameters are processed and pages reset.
- Updates the state access in ServiceDetailHeader.vue for consistency when navigating back.
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| apps/web/src/services/alert-manager/v2/stores/service-list-page-store.ts | Added searchFilters state and a new setter function to store query filters for pagination. |
| apps/web/src/services/alert-manager/v2/components/ServiceList.vue | Refactored query tag handling, pagination reset, and filter splitting logic, and removed the legacy query-tags helper. |
| apps/web/src/services/alert-manager/v2/components/ServiceDetailHeader.vue | Adjusted page number state access to use the store’s $state for consistency. |
Comments suppressed due to low confidence (3)
apps/web/src/services/alert-manager/v2/components/ServiceList.vue:218
- [nitpick] The logic for splitting 'name' filters is duplicated across the onMounted hook and the watch effect; consider refactoring it into a helper function to improve maintainability.
const splitNameFilters = searchQueryHelper.filters.map((f) => { ...
apps/web/src/services/alert-manager/v2/components/ServiceDetailHeader.vue:80
- Ensure consistent access to the store state by using 'serviceListPageStore.$state' uniformly across the validations for page numbers.
const validUnhealthyPage = (!Number.isNaN(serviceListPageStore.$state.unhealthyThisPage) && serviceListPageStore.unhealthyThisPage > 0) ? serviceListPageStore.$state.unhealthyThisPage : 1;
apps/web/src/services/alert-manager/v2/components/ServiceList.vue:64
- Avoid side effects in computed properties; verify that 'searchQueryHelper.setFilters' is a pure function when called within the computed property.
const queryTags = computed(() => searchQueryHelper.setFilters(serviceListPageStore.searchFilters).queryTags);
apps/web/src/services/alert-manager/v2/stores/service-list-page-store.ts
Show resolved
Hide resolved
skdud4659
left a comment
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~
apps/web/src/services/alert-manager/v2/stores/service-list-page-store.ts
Outdated
Show resolved
Hide resolved
Signed-off-by: 이승연 <sylee1274@mz.co.kr>
|
✅ There are no commits in this PR that require review. |
Skip Review (optional)
style,chore,ci,test,docs)Description (optional)
Things to Talk About (optional)