feat(problem): rating range filter chips for Problem Explorer#13
feat(problem): rating range filter chips for Problem Explorer#13mvanhorn wants to merge 2 commits into
Conversation
|
ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds a RatingRange enum and threads optional rating-range filtering through ProblemFilterEngine, ProblemListViewModel, and ProblemListView; UI gains rating chips and state; tests validate in-range, boundary, and nil-rating behaviors. ChangesRating Range Filter Integration
Sequence DiagramsequenceDiagram
participant User
participant ProblemListView as ProblemListView
participant ViewModel as ProblemListViewModel
participant FilterEngine as ProblemFilterEngine
participant Results as Filtered Results
User->>ProblemListView: Select rating range chip
Note over ProblemListView: selectedRatingRange updated
ProblemListView->>ProblemListView: .task(id: selectedRatingRange) triggered
ProblemListView->>ViewModel: filterProblems(query, tag, ratingRange)
ViewModel->>ViewModel: Detached async task starts
ViewModel->>FilterEngine: filter(problems, query, selectedTag, ratingRange)
Note over FilterEngine: 1. Apply text query filter<br/>2. Apply tag filter (if selectedTag)<br/>3. Apply rating filter (if ratingRange)
FilterEngine->>Results: [Problem] (filtered)
Results-->>ViewModel: Return filtered problems
ViewModel->>ViewModel: Update `@Published` filteredProblems
ViewModel-->>ProblemListView: UI re-renders with results
ProblemListView->>User: Display filtered problems
Estimated Code Review Effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
CForgeTests/ProblemFilterEngineRatingRangeTests.swift (1)
38-52: ⚡ Quick winAdd boundary-value tests to cover the half-open interval edges.
The three existing tests cover the happy path but skip the boundary values that are most likely to regress if the
contains(_:)logic changes (e.g.,>=vs>). At minimum, verify the inclusive lower bound and exclusive upper bound for one representative range.✅ Suggested boundary test
`@Test` func filterRespectsBoundaries() { let problems = [ makeProblem(id: "lower-bound", rating: 1000), // inclusive lower — must be kept makeProblem(id: "within", rating: 1100), makeProblem(id: "upper-bound", rating: 1199), // last included value makeProblem(id: "just-above", rating: 1200) // exclusive upper — must be dropped ] let filtered = ProblemFilterEngine.filter( problems: problems, query: "", selectedTag: nil, ratingRange: .r1000to1200 ) `#expect`(filtered.map { $0.id } == ["lower-bound", "within", "upper-bound"]) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@CForgeTests/ProblemFilterEngineRatingRangeTests.swift` around lines 38 - 52, Add a new unit test to cover boundary values for the half-open rating range used by ProblemFilterEngine.filter: create a `@Test` func (e.g., filterRespectsBoundaries) that builds problems with ratings 1000 (inclusive lower), 1100 (within), 1199 (last included) and 1200 (exclusive upper), call ProblemFilterEngine.filter(problems:query:selectedTag:ratingRange:) with ratingRange: .r1000to1200, and assert the filtered IDs equal ["lower-bound","within","upper-bound"]; use the existing makeProblem helper and same test file so the test exercises the inclusive lower and exclusive upper behavior.CForge/Views/Problem/ProblemListView.swift (1)
182-226: ⚡ Quick winExtract a shared chip
ViewBuilderto eliminate duplication withtagFilterBar.
ratingFilterBaris ~40 lines that reproduce the exact same background gradient,cornerRadius,overlay, andstrokemodifier chain fromtagFilterBar. Any style change (e.g., corner radius, gradient opacity) now needs updating in two places.♻️ Proposed refactor — shared chip helper
+ `@ViewBuilder` + private func filterChip(label: String, isSelected: Bool) -> some View { + Text(label) + .font(.caption) + .padding(.horizontal, 12) + .padding(.vertical, 6) + .background( + ZStack { + if isSelected { + LinearGradient( + colors: [.neonBlue, .neonPurple], + startPoint: .topLeading, + endPoint: .bottomTrailing + ) + } else { + Color.darkerBackground + } + } + ) + .foregroundColor(isSelected ? .white : .primary) + .cornerRadius(12) + .overlay( + RoundedRectangle(cornerRadius: 12) + .stroke( + LinearGradient( + colors: [.neonBlue.opacity(0.4), .neonPurple.opacity(0.4)], + startPoint: .topLeading, + endPoint: .bottomTrailing + ), + lineWidth: 1 + ) + ) + }Then in
ratingFilterBar:- Text(ratingRange.label) - .font(.caption) - .padding(.horizontal, 12) - // ... ~25 lines of styling ... + filterChip(label: ratingRange.label, + isSelected: selectedRatingRange == ratingRange)And similarly in
tagFilterBar:- Text(tag.capitalized) - .font(.caption) - .padding(.horizontal, 12) - // ... ~25 lines of styling ... + filterChip(label: tag.capitalized, + isSelected: selectedTag == tag)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@CForge/Views/Problem/ProblemListView.swift` around lines 182 - 226, The ratingFilterBar duplicates the same chip styling chain used in tagFilterBar; extract a reusable ViewBuilder (e.g., a private func chip<V: View>(_ label: String, selected: Bool, content: () -> V) -> some View or a private var/closure buildChip) that applies the shared background gradient, cornerRadius(12), overlay stroke, padding, font, foregroundColor logic and buttonStyle(.plain), then replace the per-item modifier chains in ratingFilterBar and tagFilterBar to call this helper (use RatingRange.label and selectedRatingRange in ratingFilterBar and the tag label/selection in tagFilterBar) so all visual styling is centralized.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@CForge/Views/Problem/ProblemListView.swift`:
- Line 9: The `@State` property selectedRatingRange (and the related `@State` vars
searchText and selectedTag) are flagged by SwiftLint for not being private;
either mark these properties as private (e.g., change the declarations of
selectedRatingRange, searchText, and selectedTag to private) or, if they are
intentionally internal for tests via `@testable` import, add an explicit comment
above each declaration documenting that intent and suppressing the lint rule
(e.g., a one-line comment referencing private_swiftui_state and test access) so
the linter won’t warn; locate these in the ProblemListView
(RatingRange/selectedRatingRange) and apply one consistent approach.
---
Nitpick comments:
In `@CForge/Views/Problem/ProblemListView.swift`:
- Around line 182-226: The ratingFilterBar duplicates the same chip styling
chain used in tagFilterBar; extract a reusable ViewBuilder (e.g., a private func
chip<V: View>(_ label: String, selected: Bool, content: () -> V) -> some View or
a private var/closure buildChip) that applies the shared background gradient,
cornerRadius(12), overlay stroke, padding, font, foregroundColor logic and
buttonStyle(.plain), then replace the per-item modifier chains in
ratingFilterBar and tagFilterBar to call this helper (use RatingRange.label and
selectedRatingRange in ratingFilterBar and the tag label/selection in
tagFilterBar) so all visual styling is centralized.
In `@CForgeTests/ProblemFilterEngineRatingRangeTests.swift`:
- Around line 38-52: Add a new unit test to cover boundary values for the
half-open rating range used by ProblemFilterEngine.filter: create a `@Test` func
(e.g., filterRespectsBoundaries) that builds problems with ratings 1000
(inclusive lower), 1100 (within), 1199 (last included) and 1200 (exclusive
upper), call ProblemFilterEngine.filter(problems:query:selectedTag:ratingRange:)
with ratingRange: .r1000to1200, and assert the filtered IDs equal
["lower-bound","within","upper-bound"]; use the existing makeProblem helper and
same test file so the test exercises the inclusive lower and exclusive upper
behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 857a1b1a-07f6-43de-bd37-7edd6f3d3987
📒 Files selected for processing (5)
CForge/Domain/ProblemFilterEngine.swiftCForge/Domain/RatingRange.swiftCForge/ViewModels/ProblemListViewModel.swiftCForge/Views/Problem/ProblemListView.swiftCForgeTests/ProblemFilterEngineRatingRangeTests.swift
|
Heads up: I didn't run this on a Simulator locally. The diff is small and the three Swift Testing cases for the engine cover the new code path, but you'll need to do manual QA on the chip row before merging. If anything looks off in the build, ping me and I'll iterate. |
| public enum RatingRange: String, CaseIterable, Identifiable { | ||
| case r800to1000 | ||
| case r1000to1200 | ||
| case r1200to1400 | ||
| case r1400to1800 | ||
| case r1800Plus | ||
|
|
||
| public var id: String { rawValue } |
There was a problem hiding this comment.
Nothing in this codebase uses public. All other types are internal (default access). @testable import CForge already gives the test target full internal access, so public buys you nothing here. Please remove it from the enum, id, label, and contains.
| public var label: String { | ||
| switch self { | ||
| case .r800to1000: | ||
| return "800-1000" | ||
| case .r1000to1200: | ||
| return "1000-1200" |
There was a problem hiding this comment.
The labels are misleading. contains() uses rating < 1000 (exclusive upper bound), so a problem rated exactly 1000 falls into r1000to1200, not r800to1000. A user looking at the chip "800-1000" would expect 1000 to be included.
Fix the labels to reflect the actual ranges:
case .r800to1000: return "800–999"
case .r1000to1200: return "1000–1199"
case .r1200to1400: return "1200–1399"
case .r1400to1800: return "1400–1799"
case .r1800Plus: return "1800+"
| public func contains(_ rating: Int) -> Bool { | ||
| switch self { | ||
| case .r800to1000: | ||
| return rating >= 800 && rating < 1000 | ||
| case .r1000to1200: | ||
| return rating >= 1000 && rating < 1200 | ||
| case .r1200to1400: | ||
| return rating >= 1200 && rating < 1400 | ||
| case .r1400to1800: | ||
| return rating >= 1400 && rating < 1800 |
There was a problem hiding this comment.
Quick suggestion: The 1400–1800 bucket is 400 points wide, double every other bucket (200 pts each). This is the range where most active competitive programmers sit. Consider splitting into 1400–1600 and 1600–1800 for better granularity. Not a blocker, but worth discussing.
| import Foundation | ||
|
|
||
| public enum RatingRange: String, CaseIterable, Identifiable { |
There was a problem hiding this comment.
import Foundation is unused here. RatingRange only uses Swift stdlib types (String, Bool, Int). Safe to remove.
| @Test func filterDropsNilRatedProblemsWhenRangeIsActive() { | ||
| let problems = [ | ||
| makeProblem(id: "rated", rating: 1100), | ||
| makeProblem(id: "unrated", rating: nil) | ||
| ] | ||
|
|
||
| let filtered = ProblemFilterEngine.filter( | ||
| problems: problems, | ||
| query: "", | ||
| selectedTag: nil, | ||
| ratingRange: .r1000to1200 | ||
| ) | ||
|
|
||
| #expect(filtered.map { $0.id } == ["rated"]) | ||
| } | ||
|
|
||
| @Test func filterKeepsProblemsInRange() { | ||
| let problems = [ | ||
| makeProblem(id: "in-range", rating: 1100), | ||
| makeProblem(id: "out-of-range", rating: 800) | ||
| ] | ||
|
|
||
| let filtered = ProblemFilterEngine.filter( | ||
| problems: problems, | ||
| query: "", | ||
| selectedTag: nil, | ||
| ratingRange: .r1000to1200 | ||
| ) | ||
|
|
||
| #expect(filtered.map { $0.id } == ["in-range"]) | ||
| } |
There was a problem hiding this comment.
Good coverage of the main cases, but the r1800Plus open-ended range has no test. There's no ceiling: a problem rated 3500 should still match. Please add:
@Test func filterIncludesHighRatingsInR1800Plus() {
let problems = [
makeProblem(id: "gm-level", rating: 3500),
makeProblem(id: "below", rating: 1799)
]
let filtered = ProblemFilterEngine.filter(
problems: problems, query: "", selectedTag: nil, ratingRange: .r1800Plus
)
#expect(filtered.map { $0.id } == ["gm-level"])
}
| private var ratingFilterBar: some View { | ||
| ScrollView(.horizontal, showsIndicators: false) { | ||
| HStack(spacing: 8) { | ||
| ForEach(RatingRange.allCases) { ratingRange in | ||
| Button(action: { | ||
| selectedRatingRange = selectedRatingRange == ratingRange ? nil : ratingRange | ||
| viewModel.filterProblems(query: searchText, tag: selectedTag, ratingRange: selectedRatingRange) | ||
| }) { |
There was a problem hiding this comment.
There's a double-trigger here.
The button action calls filterProblems(...) directly, and then the .task(id: selectedRatingRange) modifier below also fires when selectedRatingRange changes, so filtering runs twice per tap. The same issue exists in tagFilterBar on main, so this is consistent, but it's worth fixing in both.
The clean fix is to remove the explicit filterProblems(...) call from all button action closures and let the .task(id:) modifiers be the single source of truth:
Button(action: {
selectedRatingRange = selectedRatingRange == ratingRange ? nil : ratingRange
// ← remove filterProblems call here, .task(id: selectedRatingRange) handles it
})
| ratingFilterBar | ||
| .padding(.bottom, 8) |
There was a problem hiding this comment.
There are now two unlabeled chip rows back-to-back. First-time users won't know which row filters tags and which filters ratings. Consider adding a small section label:
Text("Rating")
.font(.caption2)
.foregroundColor(.textSecondary)
.padding(.horizontal)
ratingFilterBar
.padding(.bottom, 8)
Sandesh282
left a comment
Sandesh282
left a comment
There was a problem hiding this comment.
@mvanhorn Solid implementation overall. I have taken an initial pass. PTAL
Address all inline items from @Sandesh282 plus the coderabbitai boundary-test suggestion on PR Sandesh282#13: RatingRange.swift: - Drop unused `import Foundation`. - Remove `public` from the enum and members; codebase is internal-default and `@testable import CForge` already covers the test target. - Fix labels so they reflect the half-open intervals enforced by `contains` (800-999, 1000-1199, 1200-1399, 1400-1599, 1600-1799, 1800+). - Split the wider 1400-1800 bucket into r1400to1600 and r1600to1800 to match the 200-point granularity of the other buckets. ProblemListView.swift: - Switch the three filter `@State` properties from `internal` to `private` (existing tests drive `ProblemFilterEngine.filter` directly, not the view state, so private is safe). - Add a "Rating" caption above the rating chip row so first-time users can tell the two unlabeled chip rows apart. - Drop the explicit `filterProblems(...)` calls from the tag and rating chip button actions; the `.task(id:)` modifiers already fire when the bindings change, so the explicit calls double-triggered filtering. ProblemFilterEngineRatingRangeTests.swift: - Add `filterIncludesHighRatingsInR1800Plus` covering the open-ended upper range with a 3500-rated problem. - Add `filterIncludesInclusiveLowerBound` and `filterExcludesUpperBound` for the half-open interval boundary semantics (coderabbitai nit).
|
@Sandesh282 addressed all 8 inline items + the coderabbitai boundary-test nit in caa149d.
Tests
Verified |
2 participants
Description
Adds a horizontal row of rating range filter chips to the Problem Explorer, below the existing tag filter bar. The five buckets match the issue: 800-1000, 1000-1200, 1200-1400, 1400-1800, 1800+. Tapping a chip filters the problem list to that range. Tapping again deselects. Combines with the existing search and tag filters. Problems with a nil rating are excluded while a range is active.
The new parameter on
ProblemFilterEngine.filterdefaults to nil, so all existing call sites are unchanged.Related Issue
Closes #6
Changes Made
RatingRangeenum inCForge/Domain/RatingRange.swiftwith five cases, alabelfor chip text, andcontains(_:)for the bucket math.ProblemFilterEngine.filtergains an optionalratingRange:parameter and ANDs it after the query and tag filters.ProblemListViewModel.filterProblemsthreadsratingRangethrough to the engine.ProblemListViewadds aselectedRatingRangestate var and aratingFilterBarchip row that mirrors the tag chip styling (neon gradient on selection, capsule shape, same overlay stroke). All three filters stay in sync via.task(id:).CForgeTests/ProblemFilterEngineRatingRangeTests.swiftcover the nil, in-range, and out-of-range paths.Screenshots (if UI changes)
Will add screenshots before review.
Checklist
Cmd+B)Summary by CodeRabbit
New Features
Tests