Repository navigation
feat(gui): expose CLI recommendation filters in per-service Settings - #1094
Merged
Merged
Conversation
…lter Wire the CLI --min-count filter into the server pipeline so it can be set per service and persisted, ahead of GUI exposure. - Add ServiceConfig.MinCount (migration 000068, postgres Get/Save/List, resolver). - Apply it at read time in scheduler.filterRecsByResolvedConfigs: drop recs whose net Count is below the floor; 0 disables (matches CLI 0-no-floor). - Validate MinCount (>= 0, <= MaxServiceMinCount=10000) to stop a typo silently suppressing every recommendation. - Make updateServiceConfig presence-aware: overlay the include/exclude engine/region/type lists and min_count from the request only when the body carries the key, so a partial PUT never zeroes a filter set out-of-band. - openapi: document min_count on ServiceConfig. Regression tests: scheduler drops/keeps recs by MinCount; config rejects out-of-range MinCount; merge overlays present filters, preserves omitted ones, and clears an explicitly-empty list.
Add the missing GUI surface for the CLI recommendation-filter knobs that the
backend already persists and applies: include/exclude engines, instance types,
and regions (config.ServiceConfig, applied server-side in
scheduler.filterRecsByResolvedConfigs) plus the new per-service min-count.
- New serviceFilters.ts: pure, testable inject / populate / read helpers that
render a "Recommendation filters" panel into each Service Defaults card
(keyed off SERVICE_FIELDS, one code path for all ~20 cards). DOM is built via
createElement + .value only (no innerHTML), so user values cannot inject
markup.
- settings.ts load: inject (idempotent) + populate the controls from the
service config. save: read + validate them up front (a bad min-count aborts
the whole save with a targeted toast) and send them in the ServiceConfig
payload so they round-trip through PUT /config/service/{p}/{s}.
- api/types.ts: add ServiceConfig.min_count.
The values flow end-to-end: GUI form -> updateServiceConfig -> persisted
ServiceConfig -> scheduler read-time filter on the next recommendations fetch.
Tests: serviceFilters.test.ts (15 cases: csv parse/join, idempotent inject,
populate/read round-trip, min-count validation, no-panel fallback). Existing
182 settings tests still pass.
cristim
added
triaged
Contributor
|
Warning Review limit reached
More reviews will be available in 18 minutes and 42 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThis PR adds a complete minimum-instance-count recommendation filter from backend schema through frontend Settings UI. It introduces ChangesPer-Service Minimum-Count Recommendation Filter
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes This PR combines multiple layers (schema, persistence, API semantics, scheduler logic, and frontend UI) with presence-aware merge logic that requires careful validation and cross-layer testing. The logic density is moderate but spread across files; the primary complexity lies in the presence-aware overlay semantics in the API handler and the interaction between validation, persistence, and read-time filtering. Possibly related PRs
Suggested labels
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 |
Member
Author
|
@coderabbitai review |
Contributor
✅ Action performedReview finished.
|
Member
Author
|
@coderabbitai review Generated by Claude Code |
Contributor
|
🧠 Learnings used✅ Action performedReview finished.
|
Member
Author
|
@coderabbitai full review Generated by Claude Code |
Contributor
|
🧠 Learnings used✅ Action performedFull review finished. |
Member
Author
|
@coderabbitai full review |
Contributor
✅ Action performedFull review finished. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
frontend/src/settings.ts (1)
136-151: ⚡ Quick winConsider adding filter input IDs to dirty tracking.
The filter input IDs returned by
serviceFilterInputIds(field)are not included inTRACKED_FIELDS, so changes to recommendation filters won't trigger the "Unsaved changes" indicator. Users could edit filters, navigate away, and lose their changes without a prompt.💡 Suggested improvement
const TRACKED_FIELDS = [ 'provider-aws', 'provider-azure', 'provider-gcp', 'setting-notification-email', 'setting-auto-collect', 'setting-collection-schedule', 'setting-notification-days', 'setting-default-term', 'setting-default-payment', 'setting-default-coverage', // Per-provider grace-period inputs 'setting-grace-aws', 'setting-grace-azure', 'setting-grace-gcp', // Recommendations cycle params 'setting-recs-stale-hours', 'setting-recs-lookback-days', // Per-service fields ...SERVICE_FIELDS.map(f => f.termId), ...SERVICE_FIELDS.filter(f => f.paymentId !== null).map(f => f.paymentId as string), // Per-product SP fields (issue `#136`) ...SERVICE_FIELDS.filter(f => 'coverageId' in f).map(f => (f as { coverageId: string }).coverageId), ...SERVICE_FIELDS.filter(f => 'enabledId' in f).map(f => (f as { enabledId: string }).enabledId), + // Per-service filter inputs (PR `#1094`) + ...SERVICE_FIELDS.flatMap(f => serviceFilterInputIds(f)), ];🤖 Prompt for 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. In `@frontend/src/settings.ts` around lines 136 - 151, TRACKED_FIELDS is missing the input IDs returned by serviceFilterInputIds(field), so edits to recommendation filters aren't marked dirty; update the TRACKED_FIELDS construction to include all filter input IDs for each entry in SERVICE_FIELDS (e.g., append the results of mapping SERVICE_FIELDS through serviceFilterInputIds) so those input IDs are tracked for unsaved-change detection; ensure you reference TRACKED_FIELDS, SERVICE_FIELDS and serviceFilterInputIds when adding the mapped/flattened filter IDs.
🤖 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.
Nitpick comments:
In `@frontend/src/settings.ts`:
- Around line 136-151: TRACKED_FIELDS is missing the input IDs returned by
serviceFilterInputIds(field), so edits to recommendation filters aren't marked
dirty; update the TRACKED_FIELDS construction to include all filter input IDs
for each entry in SERVICE_FIELDS (e.g., append the results of mapping
SERVICE_FIELDS through serviceFilterInputIds) so those input IDs are tracked for
unsaved-change detection; ensure you reference TRACKED_FIELDS, SERVICE_FIELDS
and serviceFilterInputIds when adding the mapped/flattened filter IDs.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4f5c8857-ee24-4929-8541-3a473c8069cd
📒 Files selected for processing (19)
frontend/src/__tests__/serviceFilters.test.tsfrontend/src/api/types.tsfrontend/src/serviceFilters.tsfrontend/src/settings.tsinternal/api/coverage_gaps_test.gointernal/api/handler_config.gointernal/api/handler_config_test.gointernal/api/openapi.yamlinternal/config/constants.gointernal/config/resolver.gointernal/config/store_postgres.gointernal/config/store_postgres_pgxmock_test.gointernal/config/types.gointernal/config/validation.gointernal/config/validation_test.gointernal/database/postgres/migrations/000068_service_configs_min_count.down.sqlinternal/database/postgres/migrations/000068_service_configs_min_count.up.sqlinternal/scheduler/scheduler_overrides.gointernal/scheduler/scheduler_overrides_test.go
Member
Author
|
@coderabbitai review |
Contributor
✅ Action performedReview finished.
|
cristim
added a commit
that referenced
this pull request
Jun 9, 2026
Rebasing #297 onto feat/multicloud-web-frontend surfaced a migration-number collision: the base sequence already extends past 000061 (tops at 000073), and 000074/000075 are taken by in-flight PRs #1011 and #1094. Move the per-recipient mute migration to the next free slot, 000076, for both the .up.sql and .down.sql, so the merge-ref check-migration-conflicts hook stays green. Update the matching "migration 000061" reference in the config.StoreInterface doc comment.
7 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Exposes the CLI recommendation-filter knobs in the per-service Settings cards,
end to end. Closes #1093.
A. Already backend-wired -> GUI exposed
Include/exclude engines, instance types, and regions already
persist on
config.ServiceConfigand are applied server-side inscheduler.filterRecsByResolvedConfigs. This adds the GUI controls and makesthe save path persist them.
B. Newly backend-wired -> GUI exposed
Per-service min-count (the CLI
--min-countflag):ServiceConfig.MinCount(migration000068, postgres Get/Save/List,resolver).
scheduler.filterRecsByResolvedConfigs(drops recs whosenet
Countis below the floor;0disables, matching the CLI 0-no-floorsemantics).
>= 0,<= MaxServiceMinCount = 10000) so a typo can't silentlysuppress every rec.
openapi.yamlupdated.Merge path hardening
updateServiceConfig/mergeServiceConfigare now presence-aware: thefilter fields are overlaid from the request only when the JSON body carries the
key, so a partial PUT from a non-UI client never zeroes a filter set
out-of-band. The four scalar fields (enabled/term/payment/coverage) are still
overlaid unconditionally.
Frontend
New
serviceFilters.tsrenders a "Recommendation filters" panel into eachService Defaults card (one code path driven by
SERVICE_FIELDS, not ~20 HTMLblocks). DOM is built with
createElement+.valueonly (noinnerHTML), souser values cannot inject markup.
settings.tsinjects + populates on load andreads + validates on save (a bad min-count aborts the whole save with a
targeted toast).
End-to-end path verified
GUI form ->
updateServiceConfig->PUT /config/service/{p}/{s}-> persistedServiceConfig->scheduler.filterRecsByResolvedConfigson the nextrecommendations fetch. Covered for both an engine/type/region filter and the
min-count filter.
Deferred (with rationale)
max-break-even-months,min-pool-size(filter on values not on the persistedRecommendationRecord;configcan't importpkg/common),max-instances(global selection cap, not per-service), SP-types (already distinct services
with per-card enable toggles; CLI field unused),
include-extended-support(CLI-only engine-version lookups absent from the server pipeline). See
docs/code-review/open-questions/gui-filters.md. Each warrants its ownfollow-up.
Tests
presence-aware merge (overlay present / preserve omitted / clear explicit
empty).
go build ./...,go vet, andgo test ./internal/config/... ./internal/api/... ./internal/scheduler/...all green (2168 pass).
serviceFilters.test.ts(15 cases). 197 settings+filter testspass;
npx tsc --noEmitclean; production webpack build succeeds. The onlyfailing suite (
utils.test.ts, a timezone-dependentgetDatePartstest) ispre-existing on the base branch and unrelated to this change.
Summary by CodeRabbit
New Features
Tests
Chores