feat(ui/opportunities): expose AWS recommendation lookback selector (closes #909) - #911
Conversation
…loses #909) Surface the recommendation lookback window (7/30/60 days) directly in the Opportunities view, mirroring the Admin > Settings control. Both read and write the same global recommendations_lookback_days, so changing one is reflected by the other on next load. - Add a #recommendations-toolbar selector rendered by recommendations.ts, prefilled from /api/config (default 7) and gated by the update:config permission: non-admin sessions see a disabled control with a tooltip explaining why, never an editable control whose only outcome is a 403. - Tooltip wording verified against the collectors: only the AWS collector (scheduler.fetchAndConvert -> RecommendationParams.LookbackPeriod) consumes the field; Azure/GCP GetAllRecommendations take no lookback parameter. So the scope is "Applies to AWS recommendations only (Cost Explorer lookback window); Azure and GCP are unaffected." - On change: strict-integer + enum validation, persist the FULL cached config with only the lookback overridden (the backend PUT preserves only the two cycle-params when omitted, so a partial body would wipe siblings), then trigger a recommendations re-collect and reload the list. The selector is disabled while in flight; a persist failure reverts the value and toasts. - Extract the shared refresh+toast+reload block into startRecommendationsRefresh so the stale-on-open path and the lookback-change path dedup against one in-flight collect. Tests: new recommendations-lookback.test.ts (prefill, admin/non-admin gating, persist+refresh+reload, error revert). Full recommendations|settings suite green.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds a recommendations lookback selector to the Opportunities toolbar (mount point in HTML), caches full global config, enables admin-only persistence of ChangesOpportunities Lookback Selector
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly Related PRs
Suggested Labels
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
frontend/src/recommendations.ts (1)
316-343:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftA joined refresh can apply the old lookback window.
If stale-on-open already started a recollect with the previous config, Line 668 just joins that in-flight promise because Line 320 returns early. No second recollect is queued after
recommendations_lookback_daysis persisted, so the selector can show the new value while the refreshed list still reflects the old window.Also applies to: 667-669
🤖 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/recommendations.ts` around lines 316 - 343, startRecommendationsRefresh currently returns the existing autoRefreshInFlight immediately, which causes callers to "join" an in-flight recollect and miss a needed second recollect if recommendations_lookback_days changed during the in-flight refresh; modify startRecommendationsRefresh/autoRefreshInFlight handling so that when an existing autoRefreshInFlight is present you do not just return it directly but instead attach a .then() that compares the lookback value before the in-flight refresh started to the persisted recommendations_lookback_days after it finishes (or set a queueFollowUpRefresh flag when the lookback is changed), and if they differ schedule/trigger a new refresh by calling startRecommendationsRefresh(onReload) (or a forced-refresh variant) so the persisted new window is applied; use the existing symbols startRecommendationsRefresh, autoRefreshInFlight, refreshRecommendationsAPI, onReload and recommendations_lookback_days to locate and implement this logic.
🧹 Nitpick comments (1)
frontend/src/__tests__/recommendations-lookback.test.ts (1)
1-14: ⚡ Quick winDocstring claims a client-side rejection test that the suite doesn't contain.
Line 13 lists "a tampered/out-of-range value is rejected client-side" as verified behavior, but no test in this file exercises that path. Since the PR persists with strict integer + enum validation, this is a real coverage gap (a tampered
<select>value or an injected out-of-range value should be rejected beforeapi.updateConfig). Either add the test or drop the line from the docstring.Want me to draft a test that sets an out-of-range value (e.g.
90) on the select, dispatcheschange, and assertsapi.updateConfig/refreshRecsAPIare not called and the selector reverts?🤖 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__/recommendations-lookback.test.ts` around lines 1 - 14, Add a unit test to frontend/src/__tests__/recommendations-lookback.test.ts that simulates a tampered/out-of-range value on the lookback <select> (e.g. set value "90" on the rendered OpportunitiesLookback selector), dispatches a change event, and asserts that api.updateConfig and refreshRecsAPI are not called and the visible selector value reverts to the prior valid value; mock/spyon api.updateConfig and refreshRecsAPI, render the same component used by the other tests (e.g. RecommendationsLookback / OpportunitiesLookback selector), perform the DOM change + fireEvent.change, then assert no backend calls and that the DOM shows the original valid option.
🤖 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-lookback.test.ts`:
- Around line 15-19: Docstring/test mismatch: the test suite imports
loadRecommendations, resetCachedGlobalConfig, and resetAutoRefreshInFlight but
does not include a test that asserts a tampered/out-of-range lookback value is
rejected client-side as the docstring claims. Either update the docstring to
remove the “tampered/out-of-range value is rejected client-side” line, or add a
unit test that simulates setting an invalid lookback (e.g., out-of-enum or
negative value) and asserts the client-side validation prevents it (by checking
the value is not accepted, an error is thrown, or the UI/state remains
unchanged) using the existing helpers (loadRecommendations,
resetCachedGlobalConfig, resetAutoRefreshInFlight) so the new test lives
alongside the enum/option and persist-failure tests.
In `@frontend/src/recommendations.ts`:
- Around line 466-468: The admin UI allows changing the lookback when
cachedGlobalConfig is null, causing a destructive PUT that wipes other global
settings; fix by preventing save/PUT unless a full config is loaded into
cachedGlobalConfig: in loadRecommendations() and the save handler (where the PUT
occurs) check cachedGlobalConfig and if it's null either fetch the full config
via getConfig() and merge the updated lookback into that full config before PUT
or disable the save UI and surface an error until cachedGlobalConfig is
populated; update logic around cachedGlobalConfig assignment/use so the code
never falls back to an empty {} for the body of the PUT.
- Around line 618-625: The current check uses Number(rawValue) which allows
non-decimal forms (e.g. "0x3c", "7e0"); instead validate rawValue as a clean
base-10 integer string first (e.g. regex like /^[+-]?\d+$/) before converting,
then parse to a number (parseInt or Number) and verify against LOOKBACK_OPTIONS;
update the conditional around parsed (and the variables rawValue, parsed,
currentLookbackDays, and the select element 'recs-lookback-days') to reject any
input that fails the decimal-integer regex or is not included in
LOOKBACK_OPTIONS so the DOM-tamper guard matches the comment's intent.
---
Outside diff comments:
In `@frontend/src/recommendations.ts`:
- Around line 316-343: startRecommendationsRefresh currently returns the
existing autoRefreshInFlight immediately, which causes callers to "join" an
in-flight recollect and miss a needed second recollect if
recommendations_lookback_days changed during the in-flight refresh; modify
startRecommendationsRefresh/autoRefreshInFlight handling so that when an
existing autoRefreshInFlight is present you do not just return it directly but
instead attach a .then() that compares the lookback value before the in-flight
refresh started to the persisted recommendations_lookback_days after it finishes
(or set a queueFollowUpRefresh flag when the lookback is changed), and if they
differ schedule/trigger a new refresh by calling
startRecommendationsRefresh(onReload) (or a forced-refresh variant) so the
persisted new window is applied; use the existing symbols
startRecommendationsRefresh, autoRefreshInFlight, refreshRecommendationsAPI,
onReload and recommendations_lookback_days to locate and implement this logic.
---
Nitpick comments:
In `@frontend/src/__tests__/recommendations-lookback.test.ts`:
- Around line 1-14: Add a unit test to
frontend/src/__tests__/recommendations-lookback.test.ts that simulates a
tampered/out-of-range value on the lookback <select> (e.g. set value "90" on the
rendered OpportunitiesLookback selector), dispatches a change event, and asserts
that api.updateConfig and refreshRecsAPI are not called and the visible selector
value reverts to the prior valid value; mock/spyon api.updateConfig and
refreshRecsAPI, render the same component used by the other tests (e.g.
RecommendationsLookback / OpportunitiesLookback selector), perform the DOM
change + fireEvent.change, then assert no backend calls and that the DOM shows
the original valid option.
🪄 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: 84f153e7-aef9-4cde-adee-ce0425a6d6df
📒 Files selected for processing (4)
frontend/src/__tests__/recommendations-lookback.test.tsfrontend/src/index.htmlfrontend/src/recommendations.tsfrontend/src/styles/components.css
) - Block the config PUT when cachedGlobalConfig is null: if getConfig() failed during loadRecommendations(), falling back to {} on save would wipe every global setting; now toast + revert instead. - Harden integer validation: add a /^\d+$/ guard before Number() so hex ("0x3c") and exponential ("7e0") strings are rejected early, matching the feedback_strict_int_parse repo convention. - Add parameterised test covering 8 tampered/out-of-range inputs to back the docstring claim that invalid values are rejected client-side.
|
All three CR findings addressed in 31bb6a9: CRITICAL (recommendations.ts:468/639) -- config-cache race MINOR (recommendations.ts:625) -- permissive integer check MINOR (recommendations-lookback.test.ts:19) -- docstring/test mismatch @coderabbitai review |
|
Triggering a fresh review of commit (◕‿◕✿)🐇 ✅ Actions performedReview triggered.
|
Closes #909.
What
Exposes the recommendation lookback window (7 / 30 / 60 days) directly in the Opportunities view, mirroring the Admin > Settings control. Both read and write the same global
recommendations_lookback_days, so changing one is reflected by the other on next load.Verified provider scope
Grepped the recommendation collectors on
origin/feat/multicloud-web-frontend. Only the AWS collector consumes the field:scheduler.fetchAndConvertbuildsRecommendationParams.LookbackPeriodfromRecommendationsLookbackDays. The Azure and GCP collectors callGetAllRecommendations()with no lookback parameter. So the tooltip says:(Disabled non-admin variant: "Changing the lookback window requires admin (update:config) permission. Ask an administrator to adjust it in Settings.")
Behaviour
#recommendations-toolbar, prefilled from/api/config(default 7).canAccess('update', 'config')(the same permission the backend enforces). Non-admin sessions see a disabled control with an explanatory tooltip, never an editable control whose only outcome is a 403. Backend stays authoritative.recommendations_lookback_dayswould wipeenabled_providers,default_term, etc. Then it triggers a recommendations re-collect for the new window and reloads the list. The selector is disabled while in flight; a persist failure reverts the value and toasts the error (never silently leaves stale data).startRecommendationsRefreshso the stale-on-open path (feat(frontend/recs): auto-refresh on page open if >24h stale; drop freshness indicator + Refresh button #284) and the lookback-change path dedup against a single in-flight collect.Tests
New
frontend/src/__tests__/recommendations-lookback.test.ts: prefill from config, default-to-7, admin enabled + AWS-scope tooltip, non-admin (user/readonly/null) disabled + permission tooltip, change persists full config + triggers refresh + reloads, no-op when unchanged, persist failure reverts + toasts. Fullrecommendations|settingsjest suite green (515 tests). Type-check + production build clean. No backend changes.Summary by CodeRabbit