feat(settings): configurable recommendations cache-staleness threshold + provider lookback window (closes #301)#308
Conversation
…d + provider lookback window (closes #301) Add two new GlobalConfig fields: - RecommendationsCacheStaleHours: controls the stale-while-revalidate background refresh trigger (0 = disable; default 24h; max 8760h/1yr). - RecommendationsLookbackDays: AWS Cost Explorer lookback window for fresh recommendations (enum: 7, 30, or 60; default 7; GCP ignores this). Changes: - internal/config/types.go: new fields + Default*/Max*/Valid* constants - internal/config/validation.go: validateRecommendations{CacheStaleHours,LookbackDays} - internal/config/store_postgres.go: read/write new columns - internal/config/{validation,store_postgres_pgxmock}_test.go: coverage - internal/scheduler/scheduler.go: honour RecommendationsCacheStaleHours in stale-while-revalidate path; honour RecommendationsLookbackDays in fetchAndConvert - internal/scheduler/{scheduler,overrides,suppressions}_test.go: updated - internal/database/postgres/migrations/000048_*: add columns with defaults - frontend/src/{api/types,types,settings,index.html}: settings UI - frontend/src/__tests__/settings.test.ts: frontend coverage
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThis PR adds two global configuration parameters— ChangesRecommendation Cache & Lookback Configuration
Sequence DiagramsequenceDiagram
actor User
participant Frontend as Frontend UI
participant API as API Server
participant DB as Database
participant Scheduler as Scheduler
rect rgba(100, 150, 200, 0.5)
note over User,API: Config update flow
User->>Frontend: Edit recommendations settings
Frontend->>Frontend: Validate stale hours ∈ [0,8760]
Frontend->>API: POST updateConfig with<br/>recommendations_cache_stale_hours,<br/>recommendations_lookback_days
API->>DB: Save to global_config
DB-->>API: OK
API-->>Frontend: Config updated
end
rect rgba(200, 150, 100, 0.5)
note over Scheduler,DB: Recommendation listing / refresh
Scheduler->>DB: GetGlobalConfig()
DB-->>Scheduler: GlobalConfig (staleHours, lookbackDays)
alt staleHours == 0
Scheduler->>Scheduler: Skip background refresh
else
Scheduler->>Scheduler: Compute TTL from staleHours
Scheduler->>Scheduler: Check freshness vs TTL
alt stale
Scheduler->>Scheduler: maybeKickBackgroundRefresh(freshness, TTL)
end
end
alt provider == AWS
Scheduler->>Scheduler: Build lookbackPeriod from lookbackDays
Scheduler->>API: Fetch recommendations(lookbackPeriod)
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related issues
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 |
cristim
added
type/feat
|
@coderabbitai review |
✅ 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: 2
🧹 Nitpick comments (1)
internal/config/store_postgres_pgxmock_test.go (1)
58-67: ⚡ Quick winAssert the new recommendation config fields after scan.
Line 58/Line 67 and Line 104/Line 114 add the new columns to fixtures, but the tests don’t assert the scanned values. Adding assertions will better catch future SELECT/Scan-order regressions.
Proposed test hardening
@@ cfg, err := store.GetGlobalConfig(ctx) require.NoError(t, err) assert.Equal(t, []string{"aws"}, cfg.EnabledProviders) require.NotNil(t, cfg.NotificationEmail) assert.Equal(t, "ops@example.com", *cfg.NotificationEmail) + assert.Equal(t, 24, cfg.RecommendationsCacheStaleHours) + assert.Equal(t, 7, cfg.RecommendationsLookbackDays) assert.NoError(t, mock.ExpectationsWereMet()) @@ cfg, err := store.GetGlobalConfig(context.Background()) require.NoError(t, err) assert.Empty(t, cfg.GracePeriodDays) + assert.Equal(t, 24, cfg.RecommendationsCacheStaleHours) + assert.Equal(t, 7, cfg.RecommendationsLookbackDays) // GracePeriodFor returns the default for every provider. assert.Equal(t, DefaultGracePeriodDays, cfg.GracePeriodFor("aws"))Also applies to: 104-114
🤖 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 `@internal/config/store_postgres_pgxmock_test.go` around lines 58 - 67, The test adds new columns ("recommendations_cache_stale_hours", "recommendations_lookback_days") to the pgxmock fixture rows but never asserts that those values are correctly scanned; update the test that calls the Scan/Load method (the test using pgxmock.NewRows with cols and AddRow) to include explicit assertions for the scanned struct fields that map to these columns (e.g., the fields in the config struct populated by the Scan/Load function), adding checks for the expected values (24 and 7) after the Scan completes to prevent future SELECT/Scan-order regressions; apply the same additions to the duplicate fixture block around lines 104-114.
🤖 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/settings.ts`:
- Around line 2303-2310: The code uses parseInt for rawStaleHours which
truncates fractional inputs (e.g., "1.5" → 1); change the validation to parse
the input as a number and reject non-integer values: read the value from the
'setting-recs-stale-hours' element, convert with Number(value) (or parseFloat)
and then assert Number.isFinite(parsed) && Number.isInteger(parsed) && parsed >=
0 && parsed <= 8760; if the integer check fails, call showToast with the
existing error message, re-enable saveBtn, set saveInFlight = false and return
(same flow as current), updating the variable rawStaleHours to hold the integer
only when validation passes.
In `@internal/config/store_postgres.go`:
- Around line 203-204: Normalize recommendations_lookback_days before saving:
when persisting config.RecommendationsLookbackDays into global_config, treat a
zero value as unset and replace it with DefaultRecommendationsLookbackDays (the
same canonicalization that Validate() expects) so the DB never stores 0; update
the write path that uses config.RecommendationsLookbackDays to first map 0 ->
DefaultRecommendationsLookbackDays and then persist that value.
---
Nitpick comments:
In `@internal/config/store_postgres_pgxmock_test.go`:
- Around line 58-67: The test adds new columns
("recommendations_cache_stale_hours", "recommendations_lookback_days") to the
pgxmock fixture rows but never asserts that those values are correctly scanned;
update the test that calls the Scan/Load method (the test using pgxmock.NewRows
with cols and AddRow) to include explicit assertions for the scanned struct
fields that map to these columns (e.g., the fields in the config struct
populated by the Scan/Load function), adding checks for the expected values (24
and 7) after the Scan completes to prevent future SELECT/Scan-order regressions;
apply the same additions to the duplicate fixture block around lines 104-114.
🪄 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: 86d9bc91-8d31-4c76-8095-8205b2d9b5bb
📒 Files selected for processing (16)
frontend/src/__tests__/settings.test.tsfrontend/src/api/types.tsfrontend/src/index.htmlfrontend/src/settings.tsfrontend/src/types.tsinternal/config/store_postgres.gointernal/config/store_postgres_pgxmock_test.gointernal/config/types.gointernal/config/validation.gointernal/config/validation_test.gointernal/database/postgres/migrations/000048_recommendations_cycle_params.down.sqlinternal/database/postgres/migrations/000048_recommendations_cycle_params.up.sqlinternal/scheduler/scheduler.gointernal/scheduler/scheduler_overrides_test.gointernal/scheduler/scheduler_suppressions_test.gointernal/scheduler/scheduler_test.go
- settings.ts: reject fractional/non-integer stale-hours input via Number.isInteger; "1.5" now errors instead of silently truncating to 1 - store_postgres: normalise zero RecommendationsLookbackDays to DefaultRecommendationsLookbackDays before INSERT/UPDATE, matching Validate()'s semantics; RecommendationsCacheStaleHours 0 is left as-is (it is a valid "always stale" sentinel per Validate()) - pgxmock_test: assert scanned RecommendationsCacheStaleHours==24 and RecommendationsLookbackDays==7 in TestPGXMock_GetGlobalConfig_Success and the "empty json object" sub-test of GetGlobalConfig_GracePeriodDays
Pass 1 fixes
@coderabbitai review |
|
Tip For best results, initiate chat on the files or code changes.
Triggering a fresh review now. [review] |
|
@coderabbitai review (Re-ping — your previous re-review trigger from the 20:15Z ack comment doesn't seem to have produced a fresh structured review on e3991c0. CI is now all-green and the branch is mergeable. Please do a fresh pass so we can confirm zero actionable items remain.) |
|
Tip For best results, initiate chat on the files or code changes.
[review] |
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
CodeRabbit loop — exit summaryState: pass-1 findings (2 actionable + 1 nitpick) all addressed in CR feedback resolution:
Structured-review handshake note: After two trigger pings ( Ready for human merge. |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
internal/database/postgres/migrations/000048_recommendations_cycle_params.up.sql (1)
16-18: ⚡ Quick winAdd DB-level checks for the documented ranges.
These columns are documented as
0–8760and{7,30,60}, but the schema only enforcesNOT NULL DEFAULT. A manual SQL change or any unvalidated write can persist invalid values thatGetGlobalConfig()will read back verbatim and hand to the scheduler. AddingCHECKconstraints here would keep the stored config aligned with the runtime contract.Suggested migration hardening
ALTER TABLE global_config ADD COLUMN IF NOT EXISTS recommendations_cache_stale_hours INT NOT NULL DEFAULT 24, ADD COLUMN IF NOT EXISTS recommendations_lookback_days INT NOT NULL DEFAULT 7; + +ALTER TABLE global_config + ADD CONSTRAINT global_config_recommendations_cache_stale_hours_chk + CHECK (recommendations_cache_stale_hours BETWEEN 0 AND 8760), + ADD CONSTRAINT global_config_recommendations_lookback_days_chk + CHECK (recommendations_lookback_days IN (7, 30, 60));🤖 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 `@internal/database/postgres/migrations/000048_recommendations_cycle_params.up.sql` around lines 16 - 18, Add DB-level CHECK constraints on global_config to enforce the documented ranges: ensure recommendations_cache_stale_hours is between 0 and 8760 and recommendations_lookback_days is one of (7,30,60). Modify the migration that adds the two columns to also add corresponding CHECK constraints (use explicit constraint names like chk_global_config_recommendations_cache_stale_hours_range and chk_global_config_recommendations_lookback_days_allowed, and use IF NOT EXISTS semantics) so manual SQL or invalid writes cannot persist out-of-range values that GetGlobalConfig() would return to the scheduler.frontend/src/__tests__/settings.test.ts (1)
648-657: ⚡ Quick winPlease pin the fractional-hours rejection path too.
The PR notes integer-only validation for
recommendations_cache_stale_hours, but this new coverage only checks the max bound. Adding a1.5case would keep theNumber.isIntegerguard from regressing silently; ideally keep a0case alongside it to preserve the disable sentinel.Example regression test
+ test('rejects fractional recommendations_cache_stale_hours and does not call updateConfig', async () => { + (api.updateConfig as jest.Mock).mockResolvedValue({}); + (document.getElementById('setting-recs-stale-hours') as HTMLInputElement).value = '1.5'; + + const event = { preventDefault: jest.fn() } as unknown as Event; + await saveGlobalSettings(event); + + expect(api.updateConfig).not.toHaveBeenCalled(); + expect(mockShowToast).toHaveBeenCalledWith(expect.objectContaining({ kind: 'error' })); + });🤖 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/__tests__/settings.test.ts` around lines 648 - 657, Add checks to the existing test for fractional and sentinel-zero values so the integer-only guard on recommendations_cache_stale_hours can't regress: in the test that manipulates the DOM input 'setting-recs-stale-hours' and calls saveGlobalSettings, add two additional cases — set the input value to '1.5' and assert api.updateConfig was not called and mockShowToast was called with an error, and set the input value to '0' and assert the same (to preserve the disable sentinel). Use the same mocking of api.updateConfig and event object used in the current test so you target the saveGlobalSettings function and the 'setting-recs-stale-hours' input element.internal/scheduler/scheduler_test.go (1)
1064-1067: ⚡ Quick winAdd an explicit
RecommendationsCacheStaleHours == 0regression test.This PR makes
0a special disable sentinel, but the updated suite only pins positive TTLs and Lambda skip. A non-Lambda stale-cache case with DB-configured0would lock in the new “no background refresh” behavior.Possible test shape
+func TestScheduler_ListRecommendations_DisabledTTLDoesNotKickBackgroundRefresh(t *testing.T) { + ctx := context.Background() + mockStore := new(MockConfigStore) + + old := time.Now().Add(-time.Hour) + mockStore.On("GetRecommendationsFreshness", ctx). + Return(&config.RecommendationsFreshness{LastCollectedAt: &old}, nil) + mockStore.On("ListStoredRecommendations", ctx, mock.Anything). + Return([]config.RecommendationRecord{}, nil) + mockStore.On("GetGlobalConfig", ctx).Return(&config.GlobalConfig{ + RecommendationsCacheStaleHours: 0, + }, nil) + + s := &Scheduler{config: mockStore} + _, err := s.ListRecommendations(ctx, config.RecommendationFilter{}) + require.NoError(t, err) + + time.Sleep(20 * time.Millisecond) + assert.False(t, s.collecting.Load()) +}Also applies to: 1095-1098
🤖 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 `@internal/scheduler/scheduler_test.go` around lines 1064 - 1067, Add a regression test that stubs mockStore.On("GetGlobalConfig", ctx).Return(&config.GlobalConfig{RecommendationsCacheStaleHours: 0}, nil) for the non-Lambda path and assert the scheduler treats 0 as the "disabled" sentinel: verify the computed effective stale TTL is 0 (or equivalent disabled value) and that no background refresh is scheduled/executed. Place the new test alongside the existing non-Lambda cases (also add the same for the other block around lines 1095-1098), referencing mockStore, GetGlobalConfig, RecommendationsCacheStaleHours, config.GlobalConfig and the scheduler start/refresh behavior under test.
🤖 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 `@internal/config/types.go`:
- Around line 39-53: The handler's JSON decode can't distinguish omitted vs
explicit 0 for RecommendationsCacheStaleHours and RecommendationsLookbackDays,
so update the request DTO used by updateConfig (the struct decoded into in the
updateConfig handler) to use *int for those two fields
(RecommendationsCacheStaleHours and RecommendationsLookbackDays) so omission is
representable; after decoding, merge nil pointer fields by preserving the
current persisted GlobalConfig values (load existing config via whatever
Read/Get method you already have) before calling SaveGlobalConfig, and only
overwrite persisted values when the pointers are non-nil.
In `@internal/scheduler/scheduler.go`:
- Around line 807-818: The function resolveEffectiveCacheTTL dereferences
globalCfg without nil-check after calling s.config.GetGlobalConfig; if that call
returns (nil, nil) it will panic — add a nil guard: after gcErr check, if
globalCfg == nil return the existing fallback ttl and refresh enabled (i.e.,
return ttl, false); keep the existing logic for when
globalCfg.RecommendationsCacheStaleHours == 0 and for computing the duration
otherwise. Reference: resolveEffectiveCacheTTL and s.config.GetGlobalConfig and
variable globalCfg.
---
Nitpick comments:
In `@frontend/src/__tests__/settings.test.ts`:
- Around line 648-657: Add checks to the existing test for fractional and
sentinel-zero values so the integer-only guard on
recommendations_cache_stale_hours can't regress: in the test that manipulates
the DOM input 'setting-recs-stale-hours' and calls saveGlobalSettings, add two
additional cases — set the input value to '1.5' and assert api.updateConfig was
not called and mockShowToast was called with an error, and set the input value
to '0' and assert the same (to preserve the disable sentinel). Use the same
mocking of api.updateConfig and event object used in the current test so you
target the saveGlobalSettings function and the 'setting-recs-stale-hours' input
element.
In
`@internal/database/postgres/migrations/000048_recommendations_cycle_params.up.sql`:
- Around line 16-18: Add DB-level CHECK constraints on global_config to enforce
the documented ranges: ensure recommendations_cache_stale_hours is between 0 and
8760 and recommendations_lookback_days is one of (7,30,60). Modify the migration
that adds the two columns to also add corresponding CHECK constraints (use
explicit constraint names like
chk_global_config_recommendations_cache_stale_hours_range and
chk_global_config_recommendations_lookback_days_allowed, and use IF NOT EXISTS
semantics) so manual SQL or invalid writes cannot persist out-of-range values
that GetGlobalConfig() would return to the scheduler.
In `@internal/scheduler/scheduler_test.go`:
- Around line 1064-1067: Add a regression test that stubs
mockStore.On("GetGlobalConfig",
ctx).Return(&config.GlobalConfig{RecommendationsCacheStaleHours: 0}, nil) for
the non-Lambda path and assert the scheduler treats 0 as the "disabled"
sentinel: verify the computed effective stale TTL is 0 (or equivalent disabled
value) and that no background refresh is scheduled/executed. Place the new test
alongside the existing non-Lambda cases (also add the same for the other block
around lines 1095-1098), referencing mockStore, GetGlobalConfig,
RecommendationsCacheStaleHours, config.GlobalConfig and the scheduler
start/refresh behavior under test.
🪄 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: 2a75153b-b4d3-4d9d-9a77-6034e60ce691
📒 Files selected for processing (16)
frontend/src/__tests__/settings.test.tsfrontend/src/api/types.tsfrontend/src/index.htmlfrontend/src/settings.tsfrontend/src/types.tsinternal/config/store_postgres.gointernal/config/store_postgres_pgxmock_test.gointernal/config/types.gointernal/config/validation.gointernal/config/validation_test.gointernal/database/postgres/migrations/000048_recommendations_cycle_params.down.sqlinternal/database/postgres/migrations/000048_recommendations_cycle_params.up.sqlinternal/scheduler/scheduler.gointernal/scheduler/scheduler_overrides_test.gointernal/scheduler/scheduler_suppressions_test.gointernal/scheduler/scheduler_test.go
| // RecommendationsCacheStaleHours is the age (hours) at which the | ||
| // recommendations cache is considered stale and a background refresh | ||
| // fires automatically (stale-while-revalidate). 0 disables automatic | ||
| // background refresh; the cron scheduler and the manual Refresh button | ||
| // still work regardless. Valid range: 0–8760 (up to one year). | ||
| // Default: 24. | ||
| RecommendationsCacheStaleHours int `json:"recommendations_cache_stale_hours" db:"recommendations_cache_stale_hours"` | ||
|
|
||
| // RecommendationsLookbackDays is the AWS Cost Explorer lookback window | ||
| // (days) used when fetching fresh recommendations. Must be one of 7, | ||
| // 30, or 60 — the AWS Cost Explorer LookbackPeriodInDays enum. | ||
| // GCP CUD Recommender has no equivalent lookback parameter (fixed | ||
| // internally); this setting applies to AWS only. | ||
| // Default: 7. | ||
| RecommendationsLookbackDays int `json:"recommendations_lookback_days" db:"recommendations_lookback_days"` |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "SaveGlobalConfig call sites:"
rg -n -C3 '\bSaveGlobalConfig\s*\('
echo
echo "Config request decode sites:"
rg -n -C3 'Decode\(|Unmarshal\(|BindJSON\(|ShouldBindJSON\(' internal
echo
echo "Config handler/update entry points:"
rg -n -C3 '/api/config|update.*config|save.*config|global config' internalRepository: LeanerCloud/CUDly
Length of output: 50374
🏁 Script executed:
cat -n internal/api/handler_config.go | sed -n '40,75p'Repository: LeanerCloud/CUDly
Length of output: 1495
🏁 Script executed:
sed -n '120,150p' internal/config/store_postgres.goRepository: LeanerCloud/CUDly
Length of output: 1367
0 and "field omitted" are indistinguishable on these JSON ints, enabling silent overwrites.
The updateConfig handler decodes request JSON directly into a zero-valued GlobalConfig struct (line 52) and passes it straight to SaveGlobalConfig without loading the persisted config first. Since encoding/json decodes both an absent field and an explicit 0 into the same int zero value, clients can silently overwrite RecommendationsCacheStaleHours to 0 (disabling auto-refresh) or RecommendationsLookbackDays to 0 by simply omitting these fields from the request. The PR contract distinguishes omission from zero—but the transport cannot represent that distinction.
Switch the request DTO to use *int for these fields so omission is representable and preserves the persisted value, or load and merge the current config before saving.
🤖 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 `@internal/config/types.go` around lines 39 - 53, The handler's JSON decode
can't distinguish omitted vs explicit 0 for RecommendationsCacheStaleHours and
RecommendationsLookbackDays, so update the request DTO used by updateConfig (the
struct decoded into in the updateConfig handler) to use *int for those two
fields (RecommendationsCacheStaleHours and RecommendationsLookbackDays) so
omission is representable; after decoding, merge nil pointer fields by
preserving the current persisted GlobalConfig values (load existing config via
whatever Read/Get method you already have) before calling SaveGlobalConfig, and
only overwrite persisted values when the pointers are non-nil.
| func (s *Scheduler) resolveEffectiveCacheTTL(ctx context.Context) (ttl time.Duration, disabled bool) { | ||
| ttl = s.cacheTTL | ||
| globalCfg, gcErr := s.config.GetGlobalConfig(ctx) | ||
| if gcErr != nil { | ||
| // DB read failed — use the env-var / default TTL, refresh enabled. | ||
| return ttl, false | ||
| } | ||
| if globalCfg.RecommendationsCacheStaleHours == 0 { | ||
| // Explicit 0 = operator opted out of automatic background refresh. | ||
| return 0, true | ||
| } | ||
| return time.Duration(globalCfg.RecommendationsCacheStaleHours) * time.Hour, false |
There was a problem hiding this comment.
Guard the nil-config case in resolveEffectiveCacheTTL.
GetGlobalConfig is dereferenced unconditionally here. If a store implementation returns (nil, nil), ListRecommendations panics on the hot path instead of taking the existing fallback to s.cacheTTL.
Suggested hardening
func (s *Scheduler) resolveEffectiveCacheTTL(ctx context.Context) (ttl time.Duration, disabled bool) {
ttl = s.cacheTTL
globalCfg, gcErr := s.config.GetGlobalConfig(ctx)
- if gcErr != nil {
+ if gcErr != nil || globalCfg == nil {
// DB read failed — use the env-var / default TTL, refresh enabled.
return ttl, false
}
if globalCfg.RecommendationsCacheStaleHours == 0 {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func (s *Scheduler) resolveEffectiveCacheTTL(ctx context.Context) (ttl time.Duration, disabled bool) { | |
| ttl = s.cacheTTL | |
| globalCfg, gcErr := s.config.GetGlobalConfig(ctx) | |
| if gcErr != nil { | |
| // DB read failed — use the env-var / default TTL, refresh enabled. | |
| return ttl, false | |
| } | |
| if globalCfg.RecommendationsCacheStaleHours == 0 { | |
| // Explicit 0 = operator opted out of automatic background refresh. | |
| return 0, true | |
| } | |
| return time.Duration(globalCfg.RecommendationsCacheStaleHours) * time.Hour, false | |
| func (s *Scheduler) resolveEffectiveCacheTTL(ctx context.Context) (ttl time.Duration, disabled bool) { | |
| ttl = s.cacheTTL | |
| globalCfg, gcErr := s.config.GetGlobalConfig(ctx) | |
| if gcErr != nil || globalCfg == nil { | |
| // DB read failed — use the env-var / default TTL, refresh enabled. | |
| return ttl, false | |
| } | |
| if globalCfg.RecommendationsCacheStaleHours == 0 { | |
| // Explicit 0 = operator opted out of automatic background refresh. | |
| return 0, true | |
| } | |
| return time.Duration(globalCfg.RecommendationsCacheStaleHours) * time.Hour, false | |
| } |
🤖 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 `@internal/scheduler/scheduler.go` around lines 807 - 818, The function
resolveEffectiveCacheTTL dereferences globalCfg without nil-check after calling
s.config.GetGlobalConfig; if that call returns (nil, nil) it will panic — add a
nil guard: after gcErr check, if globalCfg == nil return the existing fallback
ttl and refresh enabled (i.e., return ttl, false); keep the existing logic for
when globalCfg.RecommendationsCacheStaleHours == 0 and for computing the
duration otherwise. Reference: resolveEffectiveCacheTTL and
s.config.GetGlobalConfig and variable globalCfg.
|
Tip For best results, initiate chat on the files or code changes.
CI is green, no outstanding inline comments on |
Two actionable + three nitpick items from CR review of e3991c0: Actionable: 1. handler_config.updateConfig now distinguishes "field omitted" from "explicit 0" for the two fields where 0 has a meaningful semantic (RecommendationsCacheStaleHours = 0 disables background refresh, RecommendationsLookbackDays = 0 is invalid). On a partial PUT body that omits either key, the persisted value from GetGlobalConfig is preserved instead of being silently zeroed. 2. scheduler.resolveEffectiveCacheTTL now nil-guards the config return from GetGlobalConfig. The Postgres store currently never returns (nil, nil), but a future store impl or a test mock could; the previous code would have panic-derefed. Nitpick: 3. Migration 000048 grows DB-level CHECK constraints for the documented ranges (BETWEEN 0 AND 8760, IN (7,30,60)) so a direct-SQL writer or a future broken endpoint cannot persist out-of-range values. Wrapped in a DO block + pg_constraint check for idempotency. 4. settings.test.ts pins the integer-only guard with a fractional 1.5 rejection case AND the 0-as-disable-sentinel acceptance case so neither contract regresses silently. 5. scheduler_test.go pins the disable-sentinel contract for ListRecommendations: when RecommendationsCacheStaleHours == 0, stale cached rows are still served and no background refresh fires. Existing updateConfig tests now expect a GetGlobalConfig call.
CodeRabbit pass-2 responseFixed (pushed in 6e9f0da): Actionable:
Nitpick:
@coderabbitai full review |
|
Tip For best results, initiate chat on the files or code changes.
Triggering a full review on commit [full_review] |
1 participant
Closes #301.
Two recommendation-cycle parameters are now configurable from
Settings → Generalrather than hardcoded:recommendations_cache_stale_hours(default 24) — frontend'sSTALE_THRESHOLD_MSbecomes a getter that reads the resolved global config (hours → ms). Setting0disables auto-refresh entirely (useful for demos / cost control); the existing 24-hour PR feat(frontend/recs): auto-refresh on page open when stale; drop freshness indicator + Refresh button (closes #284) #285 default kicks in if the field is unset.recommendations_lookback_days(default 7) — replaces the hardcoded"7d"ininternal/scheduler/scheduler.go. Validated to one of[7, 30, 60](matching AWS Cost Explorer'sLookbackPeriodInDaysenum, which is the most restrictive of the three providers' lookback APIs).Files
000048_recommendations_cycle_params.{up,down}.sqladds twoINT NOT NULLcolumns toglobal_configwith defaults that match the legacy hardcoded values, so existing rows pick up the right behaviour with no operator action needed.internal/config/{types,validation}.go): newGlobalConfig.RecommendationsCacheStaleHoursandRecommendationsLookbackDays. ConstantsDefault*,Max*,Valid*for the validation rules.validateRecommendationsFields()helper extracted to keep(*GlobalConfig).Validateunder gocyclo 10.internal/config/store_postgres.go): the 2 columns added to SELECT/INSERT.internal/scheduler/scheduler.go): reads the configured lookback when calling providers; reads the staleness threshold when deciding whether to kick a background refresh.resolveEffectiveCacheTTL()helper extracted to keepListRecommendationsunder gocyclo 10.frontend/src/{api/types,types,settings,index.html}.ts): UI fields under the General settings card; settings round-trip via the existing/api/configsave path (same shape as PR feat(recommendations): apply per-account overrides at read time (#196) #200'scoverageand PR feat(frontend/recs): default-select per-cell variant matching configured term + payment #223'sdefault_term/default_payment).internal/config/{validation,store_postgres_pgxmock}_test.go,internal/scheduler/scheduler{,_overrides,_suppressions}_test.go, andfrontend/src/__tests__/settings.test.ts.Notes
STALE_THRESHOLD_MSconstant name is preserved at the call site as a getter wrapper to keep the diff small; the mechanics are unchanged for callers.0-as-disable is in addition to the validation: a stored0short-circuits the auto-refresh check rather than triggering immediate refresh on every page load.Test plan
go test ./internal/config/... ./internal/api/... ./internal/scheduler/... -count=1go vet ./...gofmt -l .emptygocycloclean (helpers extracted to satisfy)tsc --noEmitcleanPR #285 referenced this work as a follow-up TODO; this closes that loop.
Summary by CodeRabbit