feat: implement bidirectional sync between search box and filter state for Issue and CL #1601
Conversation
…e for Issue and CL - Leverage additional object with filter state as single source of truth - Fix null value issue when selecting labels in Issue - Fix bug where labels are cleared after selecting assignee in Issue/CL - Comment out Search component in Issue, replace with read-only filter display - Simplify dropdown configs to directly update state
juanlou1217
changed the title
|
issue列表 list接口中的label字段后端未作处理 |
There was a problem hiding this comment.
Pull Request Overview
This PR implements a filter query display and clear functionality for the Issues page, mirroring the pattern already established in the CLView component. The changes introduce a read-only search input that displays active filters in a human-readable format (e.g., author:username assignee:username label:labelname) and provides a clear button to reset all filters.
Key Changes:
- Replaced direct label list fetching with storage-based label state management in IssuesContent
- Added filter query generation and synchronization between parent and child components
- Implemented a clear filters callback registration pattern for cross-component communication
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| moon/apps/web/components/Issues/IssuesContent.tsx | Refactored label state management, added auto-generated query logic, and registered clear filters callback |
| moon/apps/web/components/Issues/IssueSearch.tsx | Replaced search component with read-only filter display input and clear button |
| moon/apps/web/components/Issues/IssueIndex.tsx | Added filter query state management and wired up clear filters handler between parent and children |
| moon/apps/web/components/ClView/index.tsx | Refactored to use auto-generated query pattern and removed manual search query manipulation |
Comments suppressed due to low confidence (1)
moon/apps/web/components/ClView/index.tsx:490
- Similar to IssuesContent, this function has a stale closure issue. It captures
sortat the time of definition butclearAllFiltersis not wrapped inuseCallback, so it will capture stalesortvalues. Use the functional update form:setSort(prev => ({ ...prev, Author: '', Assignees: '' })).
const clearAllFilters = () => {
setSort({ ...sort, Author: '', Assignees: '' })
setLabel([])
setReview('')
}
| // const isSearchLoading = queryDebounced.length > 0 | ||
|
|
||
| const [filterQuery, setFilterQuery] = useState('') | ||
| const [onClearFilters, setOnClearFilters] = useState<(() => void) | undefined>() |
There was a problem hiding this comment.
The state type should be (() => void) | undefined without wrapping the function type. However, storing a callback function directly in state is problematic because React compares by reference. Consider using useRef to store the callback, or use a callback pattern with useCallback that doesn't require state storage.
| setSort({ ...sort, Author: '', Assignees: '' }) | ||
| setLabel([]) | ||
| }, [sort, setSort]) |
There was a problem hiding this comment.
The clearAllFilters callback has a stale closure problem. It depends on sort in the dependency array and spreads it in the callback, but setSort should not be in the dependencies (setState functions are stable). This will cause the callback to be recreated every time sort changes, and the registration effect at line 150 will re-run unnecessarily. Remove setSort from dependencies and consider using the functional update form: setSort(prev => ({ ...prev, Author: '', Assignees: '' })).
| setSort({ ...sort, Author: '', Assignees: '' }) | |
| setLabel([]) | |
| }, [sort, setSort]) | |
| setSort(prev => ({ ...prev, Author: '', Assignees: '' })) | |
| setLabel([]) | |
| }, [setSort]) |
| }, [sort, setSort]) | ||
|
|
||
| useEffect(() => { | ||
| onRegisterClearFilters?.(() => clearAllFilters) |
There was a problem hiding this comment.
This registration pattern is incorrect. You're passing a function that returns clearAllFilters instead of passing clearAllFilters directly. It should be onRegisterClearFilters?.(clearAllFilters). The current code will cause the parent to receive () => clearAllFilters (a function that returns the clear function) rather than the clear function itself.
| onRegisterClearFilters?.(() => clearAllFilters) | |
| onRegisterClearFilters?.(clearAllFilters) |
| const reviewValue = reviewMatch ? reviewMatch[1] : '' | ||
|
|
||
| const newLabelIds = | ||
| labelNames.length > 0 ? labelList.filter((l) => labelNames.includes(l.name)).map((l) => String(l.id)) : label |
There was a problem hiding this comment.
When no label names are found in the query, this falls back to the current label state. This seems semantically incorrect - if the query doesn't contain any labels, newLabelIds should probably be an empty array [] rather than preserving the old state. This could prevent labels from being cleared when they're removed from the search query.
| labelNames.length > 0 ? labelList.filter((l) => labelNames.includes(l.name)).map((l) => String(l.id)) : label | |
| labelNames.length > 0 ? labelList.filter((l) => labelNames.includes(l.name)).map((l) => String(l.id)) : [] |
| (page: number, per_page: number, additional?: AdditionType) => { | ||
| setLoading(true) | ||
| const addittion = additional ? additional : additions([]) | ||
| // 使用当前的label状态生成labelIds,避免label丢失 |
There was a problem hiding this comment.
This Chinese comment should be translated to English or provided bilingually for consistency with the rest of the codebase. Suggested: // Use current label state to generate labelIds, avoiding label loss.
| // 使用当前的label状态生成labelIds,避免label丢失 | |
| // Use current label state to generate labelIds, avoiding label loss |
2 participants
Uh oh!
There was an error while loading. Please reload this page.