Repository navigation
feat(recommendations): server-side pushdown for service/region/account filters (closes #162) - #846
Conversation
cristim
added
triaged
|
@coderabbitai review |
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 13 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThis PR implements server-side pushdown filtering for single-value ChangesService/Region Filter Pushdown
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related issues
Possibly related PRs
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)
Comment |
✅ Actions performedReview triggered.
|
![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
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@frontend/src/__tests__/recommendations.test.ts`:
- Around line 172-224: The tests set state.getRecommendationsColumnFilters via
mockReturnValue but never restore it, causing mock leakage; add an afterEach
cleanup in this test file that resets the mock implementation after each test
(e.g., call (state.getRecommendationsColumnFilters as jest.Mock).mockReset() or
mockRestore() in an afterEach block), so loadRecommendations/tests run with a
clean getRecommendationsColumnFilters mock between tests.
🪄 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: 21ee43c2-8bf0-4d98-9ac2-8930f3cc2793
📒 Files selected for processing (3)
frontend/src/__tests__/recommendations.test.tsfrontend/src/recommendations.tsinternal/api/handler_recommendations_test.go
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
@coderabbitai review |
Rate Limit Exceeded
|
|
@coderabbitai full review Generated by Claude Code |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
# Get the full PR diff
gh pr view 846 --json files,additions,deletions,title,body
echo "=== PR DIFF ==="
gh pr diff 846Length of output: 17184 🏁 Script executed: #!/bin/bash
echo "=== handler_recommendations.go ==="
cat internal/api/handler_recommendations.go
echo ""
echo "=== buildRecommendationFilter or filter builder ==="
fd -e go | xargs grep -l "buildRecommendationFilter" 2>/dev/null
echo ""
echo "=== config.RecommendationFilter struct ==="
rg -n "RecommendationFilter" --type go -C3
echo ""
echo "=== frontend api.ts RecommendationFilters ==="
fd "api.ts" frontend/src | head -5
cat frontend/src/api.ts 2>/dev/null || fd "api.ts" --type f | head -2 | xargs cat
echo ""
echo "=== frontend state.ts relevant parts ==="
rg -n "getRecommendationsColumnFilters\|RecommendationsColumnId\|ColumnFilter" frontend/src/state.ts -C2
echo ""
echo "=== IsClientError ==="
rg -n "IsClientError\|ClientError" --type go -C3Length of output: 58487 🏁 Script executed: #!/bin/bash
echo "=== validation.go ==="
fd "validation.go" --type f | head -3 | xargs cat
echo ""
echo "=== ClientError / IsClientError ==="
fd -e go | xargs grep -l "ClientError" 2>/dev/null | head -5
fd "errors.go" --type f internal/api/ | xargs cat 2>/dev/null
rg -n "ClientError\|NewClientError\|IsClientError" --type go -l | head -8 | xargs cat 2>/dev/null | head -80
echo ""
echo "=== frontend api/index.ts ==="
cat frontend/src/api/index.ts 2>/dev/null | head -20
echo ""
echo "=== frontend api RecommendationFilters ==="
fd "*.ts" frontend/src/api/ | xargs grep -l "RecommendationFilter\|getRecommendations" 2>/dev/null | head -5 | xargs cat 2>/dev/null | head -100
echo ""
echo "=== state.ts ColumnFilter / RecommendationsColumnId ==="
fd "state.ts" frontend/src/ | head -3 | xargs grep -n "ColumnFilter\|RecommendationsColumnId\|getRecommendationsColumnFilters" 2>/dev/null
echo ""
echo "=== store_postgres_recommendations.go buildRecommendationFilter ==="
cat internal/config/store_postgres_recommendations.go | grep -A 60 "func buildRecommendationFilter"Length of output: 47214
Full Review — feat(recommendations): server-side pushdown for service/region/account filtersSummaryThe overall approach is sound: single-value categorical filters for MediumMissing unit test for
If Low
This is only correct if changing the column filter for
NitsRegion SQL filter has no isolation test in
// Existing
got, err = store.ListStoredRecommendations(ctx, config.RecommendationFilter{Service: "ec2"})
// Missing
got, err = store.ListStoredRecommendations(ctx, config.RecommendationFilter{Region: "us-east-1"})
// internal/api/validation.go
if len(service) > 64 {
return NewClientError(400, "invalid service name: must be 1-64 characters")
}
if !serviceNameRegex.MatchString(service) { // ^[a-z0-9][a-z0-9-]{0,63}$
...
}The regex already caps total length at 64 ( Positive notes
🧠 Learnings used |
|
@coderabbitai full review |
✅ Action performedFull review finished. |
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
…t filters (closes #162) Wire service and region as SQL WHERE-clause parameters in ListRecommendations so the backend prunes rows before returning them. The frontend extracts a single-value categorical selection from the column-filter state and forwards it as a query param; multi-value or absent filters fall through to client-side applyColumnFilters unchanged. Account filter already triggered re-fetches via account_ids; add it to SERVER_PUSHDOWN_COLUMNS so clearing it also re-fetches. Backend: handler_recommendations.go already called validateServiceName / validateRegion / buildRecommendationFilter -- no Go changes needed beyond the test coverage added here. Frontend: commitAndRefresh() replaces the direct rerenderRecommendations() call in categorical popover commits; it re-fetches for the three pushdown columns and re-renders in-place for all others. Tests added: - Go: TestGetRecommendations_PushdownFilters (4 sub-tests covering service+region reach filter, account_id reaches filter, invalid service returns 400, invalid region returns 400) - TS: 3 new loadRecommendations tests (single-value service pushed, single- value region pushed, multi-value service not pushed)
…t order-dependent flakes
1 participant
Summary
serviceandregionas SQLWHERE-clause parameters viabuildRecommendationFilterso the backend prunes rows before returning them (issue ux(recommendations): server-side pushdown for Service / Region / numeric column filters #162)applyColumnFilterscommitAndRefresh()replaces the directrerenderRecommendations()call in categorical popover commits -- re-fetches forservice/region/accountcolumns, re-renders in-place for all othersFilters wired
service?service=ec2WHERE service = $Nregion?region=us-east-1WHERE region = $Naccountaccount_idsWHERE cloud_account_id = ANY(...)Test plan
go test github.com/LeanerCloud/CUDly/internal/api/...-- 1360 passedcd frontend && npx tsc --noEmit-- no errorscd frontend && npx jest --testPathPattern=recommendations-- 332 passedTestGetRecommendations_PushdownFilterscovers service+region reach filter, account_id reaches filter, invalid service slug returns 400, invalid region name returns 400loadRecommendationstests -- single-value service pushed, single-value region pushed, multi-value service not pushed (stays client-side)Summary by CodeRabbit