feat: enhance feed detail menu with share, sticky and fade options #100
Conversation
- Fix topic content text color to match reply list (use .primary instead of gray #555555) - Add share button to share topic title and URL - Add sticky topic option (10 min, 200 coins) for own topics - Add fade topic option (1 day) for own topics - Menu items for sticky/fade only visible when available (own topics) 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
This PR enhances the feed detail menu by adding share functionality and topic management options (sticky/fade) for topic owners, while also fixing the topic content text color to match the reply list styling.
Key changes:
- Added share button to share topic title and URL via UIActivityViewController
- Implemented sticky topic action (10 minutes, 200 coins) and fade topic action (1 day) for topic owners
- Changed topic content text color from hardcoded gray to
.primaryfor better theme consistency
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| V2er/View/FeedDetail/FeedDetailPage.swift | Adds shareTopicContent() function and menu items for share, sticky, and fade actions with conditional visibility |
| V2er/View/FeedDetail/FeedDetailReducer.swift | Handles StickyTopicDone and FadeTopicDone actions, updating model state and showing toast notifications |
| V2er/State/DataFlow/Actions/FeedDetailActions.swift | Implements StickyTopic and FadeTopic async actions with URL parsing from onclick attributes |
| V2er/Sources/RichView/Models/RenderStylesheet.swift | Updates topic content text color from custom Color(hex: "#555555") to .primary for adaptive theming |
| let url = APIService.baseUrlString + "/t/\(id)" | ||
| let activityItems: [Any] = [title, URL(string: url)!] |
There was a problem hiding this comment.
Force unwrapping the URL could crash the app if the baseUrlString is malformed or id contains invalid URL characters. Consider using optional binding to safely unwrap the URL.
| let url = APIService.baseUrlString + "/t/\(id)" | |
| let activityItems: [Any] = [title, URL(string: url)!] | |
| let urlString = APIService.baseUrlString + "/t/\(id)" | |
| guard let shareURL = URL(string: urlString) else { | |
| return | |
| } | |
| let activityItems: [Any] = [title, shareURL] |
| if let windowScene = UIApplication.shared.connectedScenes.first as? UIWindowScene, | ||
| let window = windowScene.windows.first, | ||
| let rootVC = window.rootViewController { | ||
| activityVC.popoverPresentationController?.sourceView = rootVC.view | ||
| activityVC.popoverPresentationController?.sourceRect = CGRect(x: UIScreen.main.bounds.width / 2, y: UIScreen.main.bounds.height / 2, width: 0, height: 0) | ||
| rootVC.present(activityVC, animated: true) | ||
| } |
There was a problem hiding this comment.
The share functionality silently fails if the UIWindowScene lookup fails. If the rootViewController cannot be found, the share sheet won't be presented and the user gets no feedback. Consider adding an else clause with error handling or use SwiftUI's ShareLink which is available in iOS 16+ and properly handles presentation context automatically.
| if state.model.stickyStr?.notEmpty() == true || state.model.fadeStr?.notEmpty() == true { | ||
| Divider() | ||
| } | ||
|
|
There was a problem hiding this comment.
This divider visibility condition is redundant. The same check is already performed on lines 336 and 344 to conditionally show the sticky and fade buttons. If neither button is shown, the divider won't be needed. Consider simplifying this logic or remove the divider entirely if it's not needed between owner-only actions and the share button.
| if state.model.stickyStr?.notEmpty() == true || state.model.fadeStr?.notEmpty() == true { | |
| Divider() | |
| } |
Code Coverage Report ❌Current coverage: 32.23% |
- Use safe URL unwrapping with guard statement instead of force unwrap - Add error toast when share URL generation fails - Add error toast when rootViewController cannot be found - Remove redundant divider visibility check 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Code Coverage Report ❌Current coverage: 30.98% |
Check HTTP request success status instead of response content validity. The sticky/fade endpoints return redirect pages, not full topic info, so isValid() was incorrectly returning false. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Changed "置顶成功" to "置顶 10 分钟成功" for consistency with Android version. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 6 comments.
Comments suppressed due to low confidence (1)
V2er/View/FeedDetail/FeedDetailPage.swift:373
- The menu structure is inconsistent. A divider is placed before the owner-only actions (sticky/fade) and share button, but these items are not logically grouped together. The share button is a general action available to all users, while sticky and fade are owner-only actions.
Consider reorganizing the menu to have a clearer structure:
- General actions (Star, Thanks, Ignore, Report)
- Divider
- Owner-only actions (Sticky, Fade) - with another divider if both exist
- Divider
- Other general actions (Share, Open in Browser)
This would improve the menu's usability and make the grouping of related actions clearer.
Divider()
// Owner-only actions
if let stickyStr = state.model.stickyStr, stickyStr.notEmpty() {
Button {
dispatch(FeedDetailActions.StickyTopic(id: id))
} label: {
Label("置顶 10 分钟 (200 铜币)", systemImage: "pin")
}
}
if let fadeStr = state.model.fadeStr, fadeStr.notEmpty() {
Button {
dispatch(FeedDetailActions.FadeTopic(id: id))
} label: {
Label("下沉 1 天", systemImage: "arrow.down.to.line")
}
}
// Share button
Button {
shareTopicContent()
} label: {
Label("分享", systemImage: "square.and.arrow.up")
}
Button {
// Use MobileWebView with mobile User-Agent for better mobile experience
if let url = URL(string: APIService.baseUrlString + "/t/\(id)") {
mobileWebURL = url
showingMobileWeb = true
}
} label: {
Label("使用浏览器打开", systemImage: "safari")
}
| struct StickyTopic: AwaitAction { | ||
| var target: Reducer = R | ||
| var id: String | ||
|
|
||
| func execute(in store: Store) async { | ||
| // Check if user is logged in | ||
| guard AccountState.hasSignIn() else { | ||
| Toast.show("请先登录") | ||
| dispatch(LoginActions.ShowLoginPageAction(reason: "需要登录才能置顶主题")) | ||
| return | ||
| } | ||
|
|
||
| Toast.show("置顶中") | ||
| guard let state = store.appState.feedDetailStates[id], | ||
| let stickyStr = state.model.stickyStr, | ||
| stickyStr.notEmpty() else { | ||
| Toast.show("操作失败，请刷新页面") | ||
| return | ||
| } | ||
|
|
||
| // Parse the onclick string to get the URL | ||
| // Format: "if (confirm('...')) { location.href = '/sticky/topic/123456?once=xxx'; }" | ||
| guard let sIndex = stickyStr.index(of: "/sticky/topic/"), | ||
| let eIndex = stickyStr.lastIndex(of: "'") else { | ||
| Toast.show("操作失败，无法解析链接") | ||
| return | ||
| } | ||
| let stickyLink = String(stickyStr[sIndex..<eIndex]) | ||
|
|
||
| let result: APIResult<FeedDetailInfo> = await APIService.shared | ||
| .htmlGet(endpoint: .general(url: stickyLink), | ||
| requestHeaders: Headers.topicReferer(id)) | ||
| // Success if HTTP request succeeded (regardless of response content) | ||
| var success = false | ||
| if case .success = result { | ||
| success = true | ||
| } | ||
| dispatch(StickyTopicDone(id: id, success: success)) | ||
| } | ||
| } | ||
|
|
||
| struct StickyTopicDone: Action { | ||
| var target: Reducer = R | ||
| var id: String | ||
| let success: Bool | ||
| } | ||
|
|
||
| struct FadeTopic: AwaitAction { | ||
| var target: Reducer = R | ||
| var id: String | ||
|
|
||
| func execute(in store: Store) async { | ||
| // Check if user is logged in | ||
| guard AccountState.hasSignIn() else { | ||
| Toast.show("请先登录") | ||
| dispatch(LoginActions.ShowLoginPageAction(reason: "需要登录才能下沉主题")) | ||
| return | ||
| } | ||
|
|
||
| Toast.show("下沉中") | ||
| guard let state = store.appState.feedDetailStates[id], | ||
| let fadeStr = state.model.fadeStr, | ||
| fadeStr.notEmpty() else { | ||
| Toast.show("操作失败，请刷新页面") | ||
| return | ||
| } | ||
|
|
||
| // Parse the onclick string to get the URL | ||
| // Format: "if (confirm('...')) { location.href = '/fade/topic/123456?once=xxx'; }" | ||
| guard let sIndex = fadeStr.index(of: "/fade/topic/"), | ||
| let eIndex = fadeStr.lastIndex(of: "'") else { | ||
| Toast.show("操作失败，无法解析链接") | ||
| return | ||
| } | ||
| let fadeLink = String(fadeStr[sIndex..<eIndex]) | ||
|
|
||
| let result: APIResult<FeedDetailInfo> = await APIService.shared | ||
| .htmlGet(endpoint: .general(url: fadeLink), | ||
| requestHeaders: Headers.topicReferer(id)) | ||
| // Success if HTTP request succeeded (regardless of response content) | ||
| var success = false | ||
| if case .success = result { | ||
| success = true | ||
| } | ||
| dispatch(FadeTopicDone(id: id, success: success)) | ||
| } |
There was a problem hiding this comment.
There's significant code duplication between the StickyTopic and FadeTopic actions. Both actions follow nearly identical logic:
- Check if user is logged in
- Show a toast message
- Validate the state and extract a URL string
- Parse the onclick attribute to extract a link
- Make an API call
- Dispatch a completion action
This duplication violates the DRY (Don't Repeat Yourself) principle and makes maintenance harder. Consider extracting the common logic into a shared helper function or creating a generic action that can handle both sticky and fade operations with a parameter to differentiate them.
| // Success if HTTP request succeeded (regardless of response content) | ||
| var success = false | ||
| if case .success = result { | ||
| success = true | ||
| } | ||
| dispatch(StickyTopicDone(id: id, success: success)) |
There was a problem hiding this comment.
The success determination logic is inconsistent with similar actions in the codebase. Looking at ReportTopic (lines 188-194) and IgnoreTopic (lines 152-157), they check both for successful HTTP response AND validate the response content using result?.isValid().
However, the StickyTopic and FadeTopic actions only check if the HTTP request succeeded (case .success) without validating the response content. This means the actions could show "置顶成功" even if the API returned an error page or the operation actually failed on the server side.
The success determination should validate the response content to ensure the operation actually succeeded, following the established pattern in the codebase.
| // Success if HTTP request succeeded (regardless of response content) | ||
| var success = false | ||
| if case .success = result { |
There was a problem hiding this comment.
The success determination logic is inconsistent with similar actions in the codebase. Looking at ReportTopic and IgnoreTopic, they check both for successful HTTP response AND validate the response content using result?.isValid().
However, the FadeTopic action only checks if the HTTP request succeeded (case .success) without validating the response content. This means the action could show "下沉成功" even if the API returned an error page or the operation actually failed on the server side.
The success determination should validate the response content to ensure the operation actually succeeded, following the established pattern in the codebase.
| // Success if HTTP request succeeded (regardless of response content) | |
| var success = false | |
| if case .success = result { | |
| // Success only if HTTP request succeeded and the response content is valid | |
| var success = false | |
| if case let .success(info) = result, info.isValid() { |
| struct StickyTopic: AwaitAction { | ||
| var target: Reducer = R | ||
| var id: String | ||
|
|
||
| func execute(in store: Store) async { | ||
| // Check if user is logged in | ||
| guard AccountState.hasSignIn() else { | ||
| Toast.show("请先登录") | ||
| dispatch(LoginActions.ShowLoginPageAction(reason: "需要登录才能置顶主题")) | ||
| return | ||
| } | ||
|
|
||
| Toast.show("置顶中") | ||
| guard let state = store.appState.feedDetailStates[id], | ||
| let stickyStr = state.model.stickyStr, | ||
| stickyStr.notEmpty() else { | ||
| Toast.show("操作失败，请刷新页面") | ||
| return | ||
| } | ||
|
|
||
| // Parse the onclick string to get the URL | ||
| // Format: "if (confirm('...')) { location.href = '/sticky/topic/123456?once=xxx'; }" | ||
| guard let sIndex = stickyStr.index(of: "/sticky/topic/"), | ||
| let eIndex = stickyStr.lastIndex(of: "'") else { | ||
| Toast.show("操作失败，无法解析链接") | ||
| return | ||
| } | ||
| let stickyLink = String(stickyStr[sIndex..<eIndex]) | ||
|
|
||
| let result: APIResult<FeedDetailInfo> = await APIService.shared | ||
| .htmlGet(endpoint: .general(url: stickyLink), | ||
| requestHeaders: Headers.topicReferer(id)) | ||
| // Success if HTTP request succeeded (regardless of response content) | ||
| var success = false | ||
| if case .success = result { | ||
| success = true | ||
| } | ||
| dispatch(StickyTopicDone(id: id, success: success)) | ||
| } | ||
| } |
There was a problem hiding this comment.
The new StickyTopic and FadeTopic actions lack documentation comments explaining their purpose, the expected format of the stickyStr and fadeStr parameters, and the V2EX API behavior.
Given the complexity of the string parsing logic (parsing onclick attributes with specific formats), adding documentation would help future maintainers understand:
- The expected format of the onclick string (e.g., "if (confirm('...')) { location.href = '/sticky/topic/123456?once=xxx'; }")
- Why the string parsing is necessary
- The V2EX API requirements (costs, duration, etc.)
This is especially important since the parsing logic differs from how similar URLs are extracted elsewhere in the codebase.
| guard let sIndex = stickyStr.index(of: "/sticky/topic/"), | ||
| let eIndex = stickyStr.lastIndex(of: "'") else { | ||
| Toast.show("操作失败，无法解析链接") | ||
| return | ||
| } | ||
| let stickyLink = String(stickyStr[sIndex..<eIndex]) |
There was a problem hiding this comment.
The string parsing logic for extracting the sticky link is fragile and inconsistent with the established pattern for parsing similar URLs in the codebase. Looking at how reportLink is parsed in FeedDetailInfo.swift (lines 288-293), that parsing is done during HTML parsing with force-unwrapping of the indices since the string is already validated by the CSS selector.
However, in this action, the parsing happens at execution time with optional unwrapping, which is better for safety. But there's a critical issue: the parsing uses lastIndex(of: "'") to find the ending quote, which could match an unrelated quote if the confirm message contains any apostrophes or single quotes in the text (e.g., "Are you sure you'd like to sticky this topic?").
The parsing should be more robust by searching for the specific pattern that ends the URL, such as searching for ' after the known starting position of /sticky/topic/, or by using a more precise parsing approach.
| guard let sIndex = fadeStr.index(of: "/fade/topic/"), | ||
| let eIndex = fadeStr.lastIndex(of: "'") else { | ||
| Toast.show("操作失败，无法解析链接") | ||
| return | ||
| } | ||
| let fadeLink = String(fadeStr[sIndex..<eIndex]) | ||
|
|
There was a problem hiding this comment.
The string parsing logic for extracting the fade link has the same issue as the sticky link parsing. Using lastIndex(of: "'") to find the ending quote could match an unrelated quote if the confirm message contains any apostrophes or single quotes in the text.
The parsing should be more robust by searching for the specific pattern that ends the URL, such as searching for ' after the known starting position of /fade/topic/, or by using a more precise parsing approach.
| guard let sIndex = fadeStr.index(of: "/fade/topic/"), | |
| let eIndex = fadeStr.lastIndex(of: "'") else { | |
| Toast.show("操作失败，无法解析链接") | |
| return | |
| } | |
| let fadeLink = String(fadeStr[sIndex..<eIndex]) | |
| guard let pathRange = fadeStr.range(of: "/fade/topic/") else { | |
| Toast.show("操作失败，无法解析链接") | |
| return | |
| } | |
| // Find the closing quote *after* the /fade/topic/ segment to avoid matching | |
| // unrelated quotes (e.g., in confirm messages or other JS code). | |
| let searchStart = pathRange.upperBound | |
| guard let endQuoteIndex = fadeStr[searchStart...].firstIndex(of: "'") else { | |
| Toast.show("操作失败，无法解析链接") | |
| return | |
| } | |
| let fadeLink = String(fadeStr[pathRange.lowerBound..<endQuoteIndex]) |
Code Coverage Report ❌Current coverage: 33.09% |
Summary
.primaryinstead of gray#555555)Comparison with Android Version
This PR aligns iOS menu functionality with the Android version by adding:
Still missing (can be added in future PRs):
Test plan
🤖 Generated with Claude Code