Skip to content

ux(recommendations): add SageMaker to Service filter dropdown#92

Merged
cristim merged 1 commit into
feat/multicloud-web-frontendfrom
ux/rec-service-filter-sagemaker
Apr 30, 2026
Merged

ux(recommendations): add SageMaker to Service filter dropdown#92
cristim merged 1 commit into
feat/multicloud-web-frontendfrom
ux/rec-service-filter-sagemaker

Conversation

@cristim
Copy link
Copy Markdown
Member

@cristim cristim commented Apr 26, 2026

Summary

Closes #77.

The Recommendations page Service filter dropdown is missing SageMaker, even though the AWS recommendations parser emits service=sagemaker (see providers/aws/recommendations/parser_sp.go:153, mapping to SagemakerSp). Users could see SageMaker recommendations in the unfiltered list but could not narrow to them.

Changes

  • frontend/src/index.html: Adds <option value="sagemaker">SageMaker</option> under the existing <select id="service-filter"> AWS optgroup, alphabetically positioned between redshift and savingsplans.
  • frontend/src/__tests__/html.test.ts: Regression test that parses the real index.html and asserts SageMaker is present under the AWS optgroup. Sits alongside the existing optgroup-presence test for the same dropdown.

The new option participates automatically in provider-based show/hide via the existing updateServiceFilterVisibility (recommendations.ts:87) — it iterates optgroups, no per-option wiring needed.

Why SageMaker only (not Lambda)

Test plan

  • npx jest — 1252/1252 pass across 35 suites
  • npx jest src/__tests__/html.test.ts — 95/95 pass (including the new sagemaker test)
  • npx tsc --noEmit — clean
  • npm run build — webpack production build succeeds
  • Post-deploy: open Recommendations on the deployed dashboard, select AWS provider, confirm SageMaker appears in the Service dropdown. (Verified by merge-watch after CI deploy.)

Out of scope

@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented Apr 26, 2026

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 326c665e-7ec2-4f57-83bd-abf0b1f3ecf3

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch ux/rec-service-filter-sagemaker

Comment @coderabbitai help to get the list of available commands and usage tips.

@cristim
Copy link
Copy Markdown
Member Author

cristim commented Apr 26, 2026

@coderabbitai review

@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented Apr 26, 2026

✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@cristim cristim added triaged Item has been triaged priority/p2 Backlog-worthy severity/low Minor harm urgency/this-sprint Within the current sprint impact/many Affects most users effort/xs Trivial / one-liner type/bug Defect labels Apr 28, 2026
@cristim
Copy link
Copy Markdown
Member Author

cristim commented Apr 28, 2026

Merge conflict detected (mergeStateStatus: DIRTY) — rebase onto feat/multicloud-web-frontend needed. Otherwise ready: CI green, one-line fix for the missing SageMaker filter option that closes #77. (triage agent wave2-E)

@cristim
Copy link
Copy Markdown
Member Author

cristim commented Apr 29, 2026

Heads-up: this PR may be obsolete.

Attempted a rebase onto current feat/multicloud-web-frontend while clearing the queue. The rebase aborted with a semantic conflict in frontend/src/index.html at the <select id="service-filter"> block.

Root cause: base commit 034ea00cf (feat(recommendations): per-column header filters + sticky bottom action box) deleted the entire recommendations-controls section that hosted the dropdown this PR was extending. The base now also has explicit negative-guard tests (frontend/src/__tests__/html.test.ts:154-166) asserting #service-filter, #region-filter, #min-savings-filter, etc. are null post-Bundle-B.

Re-applying this PR (which re-adds the deleted <select id="service-filter">) would intentionally violate those regression tests.

If SageMaker is missing from the new column-header Service popover, that's a separate, smaller fix in recommendations.ts (column-filter primitives). Suggest closing this PR and filing a fresh issue if needed.

The AWS recommendations parser emits `sagemaker` as a distinct service
(see providers/aws/recommendations/parser_sp.go:153, mapping to
SagemakerSp), but the Recommendations page Service filter dropdown
omitted it — users could see SageMaker recommendations in the unfiltered
list but could not narrow to them.

Adds `<option value="sagemaker">SageMaker</option>` under the AWS
optgroup in the static `<select id="service-filter">`, alphabetically
positioned between `redshift` and `savingsplans`. The option
participates automatically in provider-based show/hide via the existing
`updateServiceFilterVisibility` (it iterates optgroups, no per-option
wiring needed).

Lambda is intentionally NOT added in this PR:

- The AWS recommendations parser does not emit `service=lambda`. Lambda
  commitments are surfaced under the existing `savingsplans` (Compute
  Savings Plans) umbrella, which the dropdown already exposes.
- SageMaker Savings Plans is a distinct AWS product from Compute
  Savings Plans (different commitment scope + pricing), which is why
  SageMaker warrants its own filter value while Lambda does not.
- This decision is correct under both possible resolutions of the
  still-open PR #71 (separate purchasing-defaults cards) and the
  already-merged PR #53 (Savings Plans is the umbrella for SM/Lambda).

Adds a regression test in `frontend/src/__tests__/html.test.ts` that
parses the real index.html and asserts SageMaker is present under the
AWS optgroup, alongside the existing optgroup-presence test.

Verification:
- `npx jest` — 1252/1252 pass across 35 suites
- `npx tsc --noEmit` — clean
- `npm run build` — webpack production build succeeds

Closes #77
@cristim cristim force-pushed the ux/rec-service-filter-sagemaker branch from 9b30879 to f439463 Compare April 30, 2026 05:21
@cristim
Copy link
Copy Markdown
Member Author

cristim commented Apr 30, 2026

Repurposed this PR from the old top-bar Service dropdown change into a regression test for the current recommendations UI.

The old dropdown was removed upstream; the current implementation builds service filters from the column-header popover, so I added a test that seeds a SageMaker recommendation and asserts the Service popover includes it.

Branch has been force-updated to the new single-commit diff.

@cristim cristim merged commit 0653250 into feat/multicloud-web-frontend Apr 30, 2026
4 checks passed
@cristim cristim deleted the ux/rec-service-filter-sagemaker branch April 30, 2026 13:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

effort/xs Trivial / one-liner impact/many Affects most users priority/p2 Backlog-worthy severity/low Minor harm triaged Item has been triaged type/bug Defect urgency/this-sprint Within the current sprint

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant