feat: add reply sorting by popularity #121
Conversation
- Add ReplySortType enum with byTime and byPopularity options - Add loveCount computed property to parse like counts from reply items - Add segmented control UI to toggle between time and popularity sorting - Sort replies by like count (descending), with floor number as tiebreaker - Support dark/light mode color adaptation for the segment control 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]>
There was a problem hiding this comment.
Pull request overview
This PR adds the ability to sort topic replies by popularity (like count) in addition to the default chronological ordering. Users can toggle between "时间" (time/floor order) and "热门" (popularity/likes) sorting using a new segmented control in the reply section header.
Key Changes:
- Added
ReplySortTypeenum to define sorting modes with display names and icons - Implemented
loveCountcomputed property to parse numeric like counts from reply data - Added segmented control UI in reply section header with proper dark/light mode color adaptation
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| V2er/State/DataFlow/State/FeedDetailState.swift | Defines ReplySortType enum and adds replySortType state property to track current sort mode |
| V2er/State/DataFlow/Model/FeedDetailInfo.swift | Adds loveCount computed property to extract numeric value from the love string field for sorting |
| V2er/View/FeedDetail/FeedDetailPage.swift | Implements sortedReplies computed property for sorting logic and replySectionHeader UI with segmented control |
| ForEach(ReplySortType.allCases, id: \.self) { sortType in | ||
| Button { | ||
| withAnimation(.easeInOut(duration: 0.2)) { | ||
| store.appState.feedDetailStates[instanceId]?.replySortType = sortType |
There was a problem hiding this comment.
Direct state mutation is used here for UI-only state. While the project follows a Redux-like architecture where state changes typically flow through actions and reducers (see CLAUDE.md), there are a few existing instances of direct mutation for simple UI state (e.g., line 310 in this file for replyContent, line 28 in ExplorePage.swift for scrollTopTab).
However, for consistency and maintainability, consider whether this UI state should be a local @State variable instead (similar to how MyFavoritePage.swift uses @State private var selectedTab for its segmented control), or follow the Redux pattern with an action and reducer case. This would make the state management pattern more consistent and easier to understand.
| // 提取数字部分 | ||
| let numbers = trimmed.components(separatedBy: CharacterSet.decimalDigits.inverted) | ||
| .joined() | ||
| return Int(numbers) ?? 0 |
There was a problem hiding this comment.
The parsing logic for loveCount may not correctly handle all possible formats of the love field. The current implementation uses components(separatedBy: CharacterSet.decimalDigits.inverted).joined() which will concatenate all digit sequences together.
For example, if love is "♥ 1 2 3", this would return "123" instead of extracting just the first number. While the V2EX format typically shows "♥ 3" or just "3", it's safer to extract only the first number sequence to avoid unexpected behavior.
Consider using a more robust approach like trimmed.components(separatedBy: CharacterSet.decimalDigits.inverted).first(where: { !$0.isEmpty }) or a regex pattern to extract the first numeric value.
| // 提取数字部分 | |
| let numbers = trimmed.components(separatedBy: CharacterSet.decimalDigits.inverted) | |
| .joined() | |
| return Int(numbers) ?? 0 | |
| // 提取第一个连续的数字片段，避免将多个数字段拼接在一起 | |
| let numberString = trimmed | |
| .components(separatedBy: CharacterSet.decimalDigits.inverted) | |
| .first(where: { !$0.isEmpty }) ?? "" | |
| return Int(numberString) ?? 0 |
Code Coverage Report ❌Current coverage: 32.36% |
Address Copilot review feedback: use first(where:) instead of joined() to extract only the first digit sequence, avoiding concatenation of multiple number segments. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]>
Code Coverage Report ❌Current coverage: 30.75% |
- Remove animation when switching sort type for instant feedback - Use floor as stable id in ForEach to prevent list jumping on sort change 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]>
| ? Color.tintColor | ||
| : Color.clear | ||
| ) | ||
| } |
There was a problem hiding this comment.
The segmented control buttons should use .buttonStyle(.plain) to prevent default button highlighting behavior in iOS. Without this, tapping a button may show unwanted highlighting effects that conflict with the custom selected state styling. Add .buttonStyle(.plain) after the Button's label closure (line 353).
| } | |
| } | |
| .buttonStyle(.plain) |
|
|
||
| // Reply Section | ||
| ForEach(state.model.replyInfo.items) { item in | ||
| ForEach(sortedReplies, id: \.floor) { item in |
There was a problem hiding this comment.
Using floor as the ForEach identifier instead of the default id (UUID) property will cause SwiftUI view identity issues when sorting changes. When the sort order toggles between time and popularity, SwiftUI will incorrectly reuse views because the floor numbers don't change, leading to:
- Animation glitches or no animations
- Potential view state persistence issues (e.g., expanded states, gestures)
- Views not properly recreating when they should
The Item struct is already Identifiable with a UUID id property. Keep using the default identifier by changing this line back to ForEach(sortedReplies) without the id: parameter.
| ForEach(sortedReplies, id: \.floor) { item in | |
| ForEach(sortedReplies) { item in |
Code Coverage Report ❌Current coverage: 28.78% |
Summary
Changes
ReplySortTypeenum andreplySortTypestateloveCountcomputed property to parse like countsTest plan
🤖 Generated with Claude Code