fix: block list filters not clearing #1006
Conversation
WalkthroughThe pull request focuses on refactoring the filter management in block-related Vue components, specifically Changes
Possibly related PRs
Suggested reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
github-actions
bot
added
bug
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
packages/plugins/materials/src/meta/block/src/BlockGroupFilters.vue (1)
67-69: Consider adding JSDoc or English commentsWhile the Chinese comment clearly explains the structure, consider adding JSDoc or English comments for better international collaboration.
const state = reactive({ - // filterValues 是一个对象。key 为 filter id，value 为选中的值，是一个数组 + /** + * Filter values object where: + * - key: filter ID + * - value: array of selected values + */ filterValues: initFilterValues() })packages/plugins/materials/src/meta/block/src/BlockGroupPanel.vue (1)
Line range hint
196-218: Consider adding loading state for better UXThe search implementation could benefit from a loading state to provide feedback during the async operation.
const state = reactive({ searchValue: '', blockList: [], + isSearching: false, filters: [ // ... ], filterValues: {} }) const searchBlocks = (filters) => { state.filterValues = filters + state.isSearching = true nextTick(() => { const params = { // ... } fetchAvailableBlocks(params) .then((data) => { state.blockList = selectedBlockFilter(data) }) .catch((error) => { message({ message: `区块搜索失败: ${error.message || error}`, status: 'error' }) }) + .finally(() => { + state.isSearching = false + }) }) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/plugins/materials/src/meta/block/src/BlockGroupFilters.vue(4 hunks)packages/plugins/materials/src/meta/block/src/BlockGroupPanel.vue(7 hunks)
🔇 Additional comments (7)
packages/plugins/materials/src/meta/block/src/BlockGroupFilters.vue (3)
8-10: LGTM! Improved state management for filtersThe changes centralize filter state management and simplify event handling by using a single source of truth (
filterValues).Also applies to: 21-25
57-65: LGTM! Well-structured initialization logicThe
initFilterValuesfunction provides a clean and consistent way to initialize filter states.
86-91: LGTM! Clear and focused filter managementThe
handleValueChangeandclearFiltersfunctions provide a clean implementation for managing and resetting filter states, directly addressing the PR objective.packages/plugins/materials/src/meta/block/src/BlockGroupPanel.vue (4)
12-16: LGTM! Improved component integrationThe template changes properly integrate with the new filter management system, allowing programmatic control of filters through refs.
Also applies to: 23-23
113-114: LGTM! Consistent state managementThe addition of
filterValuesto state maintains consistency with the filter component's state management approach.
258-270: LGTM! Robust panel state managementThe watch implementation effectively handles panel visibility and group ID changes, with proper edge case handling through early returns. This directly addresses the PR objective of clearing filters when the panel is reopened.
119-123: Verify error handling for edge casesThe
clearSearchParamsfunction effectively clears both search and filter states. Consider adding error handling for edge cases wheregroupFiltersRefmight be in an unexpected state.
…to fix/block-list-filter-not-clearing
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
packages/plugins/materials/src/meta/block/src/BlockGroupPanel.vue (3)
119-123: Consider adding error handling for filter clearing.While the implementation is good, consider adding error handling to ensure robustness:
const clearSearchParams = () => { state.searchValue = '' state.filterValues = {} - groupFiltersRef.value?.clearFilters() + try { + groupFiltersRef.value?.clearFilters() + } catch (error) { + console.error('Failed to clear filters:', error) + } }
Line range hint
196-216: Enhance error handling in searchBlocks.While the function logic is good, consider improving error handling:
const searchBlocks = (filters) => { state.filterValues = filters nextTick(() => { const params = { groupId: selectedGroup.value.groupId, label_contains: state.searchValue.trim(), tag: filters?.tag, publicType: filters?.publicType, author: filters?.author } fetchAvailableBlocks(params) .then((data) => { state.blockList = selectedBlockFilter(data) }) .catch((error) => { + console.error('Search blocks failed:', error) message({ - message: `区块搜索失败: ${error.message || error}`, + message: `区块搜索失败: ${error?.message || '未知错误'}`, status: 'error' }) + state.blockList = [] // Reset list on error }) }) }
258-270: LGTM! Watch logic handles panel state changes correctly.The implementation properly clears filters when the panel is shown. Consider adding a comment explaining the importance of the operation sequence.
watch([() => panel.show, () => selectedGroup.value.groupId], ([show, groupId]) => { if (!show) { return } if (!groupId || isDefaultGroupId(groupId)) { return } + // Clear filters before fetching blocks to ensure a fresh state panelState.isBlockGroupPanel = true clearSearchParams() fetchBlocks(groupId) getFilters() })
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/plugins/materials/src/meta/block/src/BlockGroupPanel.vue(8 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: push-check
🔇 Additional comments (3)
packages/plugins/materials/src/meta/block/src/BlockGroupPanel.vue (3)
12-24: LGTM! Template changes improve filter management.The addition of
refand modification of the search event handler align well with the PR objective of fixing filter clearing issues.
36-36: LGTM! State management setup is well-structured.The addition of
filterValuesandgroupFiltersRefprovides a clean way to manage filter state.Also applies to: 113-114, 117-117
119-123: Verify that filters clear in all scenarios.The implementation looks solid for clearing filters when:
- Adding blocks
- Closing the panel
- Changing group ID
- Reopening the panel
However, please verify these scenarios in testing:
Also applies to: 258-270
✅ Verification successful
Filter clearing implementation verified successfully
The codebase shows a clean and consistent implementation of filter clearing functionality, with proper coordination between
clearFilters()andclearSearchParams(). No conflicting implementations or known issues were found.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for any remaining filter-related TODOs or FIXMEs echo "Checking for filter-related TODOs/FIXMEs in block management code..." rg -i "todo|fixme|hack|xxx|bug" --glob "packages/plugins/materials/src/meta/block/**" echo "Checking for other filter clearing implementations that might conflict..." rg "clearFilters|clearSearchParams|resetFilters|resetSearch" --glob "packages/plugins/materials/src/meta/block/**"Length of output: 1318
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
packages/plugins/materials/src/meta/block/src/BlockGroupPanel.vue (2)
Line range hint
135-167: Consider enhancing error handling.While the success path correctly clears search parameters, consider also clearing them in the error case to maintain consistent state.
.catch((error) => { + clearSearchParams() message({ message: `添加区块失败: ${error.message || error}`, status: 'error' }) })
205-224: Consider adding input validation.While the search functionality is well-implemented, consider adding validation for the filter values to prevent potential issues with undefined or invalid filter values.
const searchBlocks = (filters) => { + if (!filters || typeof filters !== 'object') { + filters = {} + } state.filterValues = filters const params = { groupId: selectedGroupId.value, label_contains: state.searchValue.trim(), - tag: filters?.tag, - publicType: filters?.publicType, - author: filters?.author + tag: filters.tag || undefined, + publicType: filters.publicType || undefined, + author: filters.author || undefined } // ... }packages/plugins/materials/src/meta/block/src/BlockGroupFilters.vue (2)
8-10: LGTM! Consider adding type safety for filterValues.The centralized approach to filter state management using
filterValuesis a good improvement. The consistent event handling across both checkbox and select components enhances maintainability.Consider adding TypeScript or PropTypes to ensure type safety for the
filterValuesobject structure:+ type FilterValues = Record<string, string[]>; const state = reactive({ - filterValues: initFilterValues() + filterValues: initFilterValues() as FilterValues });Also applies to: 21-25
67-69: Translate Chinese comment to English for better maintainability.The implementation looks good, but let's maintain consistency in documentation.
const state = reactive({ - // filterValues 是一个对象。key 为 filter id，value 为选中的值，是一个数组 + // filterValues is an object where key is the filter id and value is an array of selected values filterValues: initFilterValues() })
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/plugins/materials/src/meta/block/src/BlockGroupFilters.vue(4 hunks)packages/plugins/materials/src/meta/block/src/BlockGroupPanel.vue(7 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: push-check
🔇 Additional comments (8)
packages/plugins/materials/src/meta/block/src/BlockGroupPanel.vue (7)
12-17: Well-structured component lifecycle management!The addition of
v-if="panel.show"and:key="selectedGroupId"ensures proper component cleanup and recreation, which effectively supports filter clearing when the panel is reopened or when the group changes.
114-115: Good state management implementation!The addition of
filterValuesto the reactive state andselectedGroupIdas a ref provides proper reactivity for managing filter states.Also applies to: 118-118
120-127: Consider handling default group ID case.The watch handler only updates
selectedGroupIdfor non-default group IDs. Consider whether the default group ID case needs special handling or documentation.Could you clarify what behavior is expected when a default group ID is selected? This might help prevent potential edge cases.
129-132: Perfect implementation for the bug fix!The
clearSearchParamsfunction properly addresses the reported bug by resetting both the search value and filter values when needed.
190-190: Good cleanup implementation!Calling
clearSearchParamsincloseGroupPanelensures proper state cleanup when the panel is closed.
266-274: Well-structured watch implementation!The watch on both
panel.showandselectedGroupIdwith proper cleanup and re-fetching logic ensures consistent state management.
279-279: Proper exposure of required state!The addition of
selectedGroupIdto the component's exports properly supports the new functionality.packages/plugins/materials/src/meta/block/src/BlockGroupFilters.vue (1)
57-65: Verify filter clearing functionality.The implementation looks good. The
initFilterValuesfunction properly initializes empty arrays for each filter, andhandleValueChangecorrectly emits the updated values. This should fix the issue where filters weren't clearing properly.Let's verify that there are no other components directly manipulating these filter values:
Also applies to: 86-87
✅ Verification successful
Filter clearing functionality is properly implemented and coordinated
The verification confirms that filter values are handled correctly across components:
- BlockGroupFilters.vue maintains the source of truth with proper initialization
- BlockGroupPanel.vue only uses filter values for search functionality without interfering with the clearing mechanism
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any direct mutations of filterValues in other components rg -l "filterValues.*=" packages/plugins/materials/src/meta/block/src/Length of output: 138
Script:
#!/bin/bash # Check the implementation in BlockGroupPanel.vue rg "filterValues" packages/plugins/materials/src/meta/block/src/BlockGroupPanel.vue -A 5 -B 5Length of output: 1226
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
packages/plugins/materials/src/meta/block/src/BlockGroupPanel.vue (4)
6-11: Consider using v-show instead of v-if for block-group-filtersThe
v-if="panel.show"directive will cause the component to be destroyed and recreated, which might be unnecessary since the panel's visibility is already controlled by the parentplugin-setting. Usingv-showcould be more performant if the panel is frequently toggled.- <block-group-filters - v-if="panel.show" + <block-group-filters + v-show="panel.show"
116-117: Initialize filterValues with an empty objectConsider initializing
filterValueswhen declaring the state to ensure consistent type and prevent potential undefined errors.], - filterValues: {} + filterValues: Object.freeze({})
122-129: Add error handling to the watch callbackThe watch callback for
selectedGroup.value.groupIdshould include error handling to prevent potential runtime errors ifselectedGroup.valueis undefined.watch( () => selectedGroup.value.groupId, (groupId) => { + try { if (groupId && !isAllGroupId(groupId) && !isDefaultGroupId(groupId)) { validGroup.value = { ...selectedGroup.value } } + } catch (error) { + console.error('Error updating validGroup:', error) + } } )
131-134: Add validation to clearSearchParamsThe
clearSearchParamsmethod should validate state properties before clearing them to prevent potential runtime errors.const clearSearchParams = () => { + if (!state) return state.searchValue = '' state.filterValues = {} }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/plugins/materials/src/meta/block/src/BlockGroupPanel.vue(8 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: push-check
🔇 Additional comments (1)
packages/plugins/materials/src/meta/block/src/BlockGroupPanel.vue (1)
14-19: Verify search behavior with empty filtersThe search functionality now includes filter values in the search operation. Ensure that the search works correctly when no filters are selected.
✅ Verification successful
Search functionality correctly handles empty filters
The implementation properly manages empty filters by:
- Initializing filterValues as an empty object
- Allowing search with just the search term via label_contains
- Including proper filter state reset functionality
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for empty filter handling ast-grep --pattern 'searchBlocks($_)'Length of output: 39
Script:
#!/bin/bash # Search for searchBlocks function implementation with context ast-grep --pattern 'searchBlocks($$$) { $$$ }' # Also search with ripgrep for any references rg "searchBlocks" -A 5 # Look for filter-related state management rg "filterValues" -A 3Length of output: 5488
…to fix/block-list-filter-not-clearing
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/plugins/materials/src/meta/block/src/BlockGroupPanel.vue (1)
206-225: Add input validation to searchBlocksThe function should validate its inputs to handle edge cases more robustly.
const searchBlocks = (filters) => { + if (!filters) { + filters = {} + } state.filterValues = filters const params = { groupId: validGroup.value.groupId, - label_contains: state.searchValue.trim(), + label_contains: (state.searchValue || '').trim(), tag: filters?.tag, publicType: filters?.publicType, author: filters?.author } + if (!params.groupId) { + console.warn('Invalid groupId, skipping search') + return + } fetchAvailableBlocks(params)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/plugins/materials/src/meta/block/src/BlockGroupPanel.vue(9 hunks)
🧰 Additional context used
📓 Learnings (1)
packages/plugins/materials/src/meta/block/src/BlockGroupPanel.vue (1)
Learnt from: gene9831
PR: opentiny/tiny-engine#1006
File: packages/plugins/materials/src/meta/block/src/BlockGroupPanel.vue:268-276
Timestamp: 2025-01-10T03:56:47.308Z
Learning: In the BlockGroupPanel component of tiny-engine, `fetchBlocks(groupId)` and `getFilters()` are independent operations. `fetchBlocks` retrieves the initial block list with empty filter parameters, while `getFilters` populates the filter UI options. They can be called in parallel without waiting for each other.
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: push-check
🔇 Additional comments (7)
packages/plugins/materials/src/meta/block/src/BlockGroupPanel.vue (7)
6-10: Good use of :key for component reactivity!The
:key="validGroup.groupId"binding ensures proper component re-rendering when the group changes.
115-116: Good state management practice!Centralizing filter values in the reactive state object improves maintainability and state tracking.
119-128: Well-implemented group validation logic!The watch handler properly validates group changes and maintains a separate valid group state.
130-133: Good refactoring of reset logic!Centralizing the reset logic in
clearSearchParamsimproves code maintainability and reduces duplication.
267-275: Correct implementation of independent operations!The parallel execution of
fetchBlocksandgetFiltersis appropriate as they are independent operations - one fetches the initial block list while the other populates filter options.
Line range hint
302-309: Clean and well-scoped styling!Good use of scoped styles and deep selector for targeting child component elements.
17-17: Verify search behavior with empty filter valuesThe search handler doesn't check for undefined/null filter values which could cause issues.
-@update:modelValue="searchBlocks(state.filterValues)" +@update:modelValue="searchBlocks(state.filterValues || {})"
English | 简体中文
PR
PR Checklist
Please check if your PR fulfills the following requirements:
PR Type
What kind of change does this PR introduce?
Background and solution
What is the current behavior?
物料插件-区块分组-添加区块，勾选了几个区块列表筛选条件，然后添加区块。添加区块成功后，再次点击添加区块，此时发起的请求是请求全部可添加的区块，而筛选条件未清空
What is the new behavior?
点击添加区块后，展示的区块列表面板，关闭后重新打开，或者更换了分组id，都会重新请求全部可添加的区块，此时应该清除所有筛选条件
Does this PR introduce a breaking change?
Other information
与PR#995修改冲突，解决冲突时发现PR#995修改会造成搜索bug（先选择作者再输入搜索字符，与先输入搜索字符再选择作者结果不一致，根因是把字符筛选改成了前端检索），PR#995回退PR[#1015 ]
Summary by CodeRabbit
Summary by CodeRabbit
New Features
Improvements
Bug Fixes