feat(frontend): refresh button shows in-flight state + toasts#52
Conversation
Rename the shared freshness Refresh button to "Refreshing..." while the POST /api/recommendations/refresh call is in flight, and surface a sticky info toast on start + a 5s success toast on completion (or an error toast on failure). The freshness bar is re-rendered on success, which naturally updates the "Data from <relative-time>" timestamp. Adds two freshness.test.ts cases: one that holds the refresh promise open to assert the in-flight button text/disabled state + info toast, and one that asserts the error path restores the button and emits an error toast.
|
@coderabbitai review |
|
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:
📝 WalkthroughWalkthroughThe refresh button handler in the freshness component now provides toast-based user feedback during refresh operations, including button state management (disabled/enabled, label changes) and error handling with appropriate toast messages displayed to the user. Changes
Sequence DiagramsequenceDiagram
actor User
participant Button as Refresh Button
participant Handler as Click Handler
participant Toast as Toast System
participant API as Recommendations API
User->>Button: Click refresh button
Button->>Handler: Trigger click event
Handler->>Button: Disable & set label to "Refreshing..."
Handler->>Toast: Show info toast "Refreshing recommendations…"
Handler->>API: Call refreshRecommendations()
API-->>Handler: API resolves (success or error)
alt Success Path
Handler->>Toast: Dismiss info toast
Handler->>Toast: Show success toast "Recommendations refreshed"
Handler->>Button: Re-enable & restore original label
else Error Path
Handler->>Toast: Dismiss info toast
Handler->>Toast: Show error toast with error message
Handler->>Button: Re-enable & restore original label
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 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 unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 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
🧹 Nitpick comments (1)
frontend/src/freshness.ts (1)
186-188: Optional: use theHTMLButtonElementproperty setters.Now that
btnis typed asHTMLButtonElement,btn.disabled = true/btn.disabled = falseis more idiomatic thansetAttribute('disabled', 'true')+removeAttribute('disabled'), andtextContenton an element is nevernullso the?? 'Refresh'fallback on Line 186 is dead. Feel free to defer — behaviourally equivalent.♻️ Suggested tidy-up
- const originalText = btn.textContent ?? 'Refresh'; - btn.setAttribute('disabled', 'true'); + const originalText = btn.textContent; + btn.disabled = true; btn.textContent = 'Refreshing...'; @@ - btn.removeAttribute('disabled'); - btn.textContent = originalText; + btn.disabled = false; + btn.textContent = originalText;Note: the existing success-path test asserts
getAttribute('disabled') === 'true'(Line 147 of the test), which would need to becomeexpect(btn.disabled).toBe(true)to match.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/freshness.ts` around lines 186 - 188, The code unnecessarily uses setAttribute/removeAttribute and a dead null-coalescing fallback on textContent: since btn is an HTMLButtonElement, replace btn.setAttribute('disabled','true') / btn.removeAttribute('disabled') with btn.disabled = true / btn.disabled = false, remove the redundant "?? 'Refresh'" fallback when reading btn.textContent, and update the corresponding test assertion (which currently checks getAttribute('disabled') === 'true') to assert btn.disabled === true; target the btn variable usage in freshness.ts and the test that asserts disabled.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@frontend/src/freshness.ts`:
- Around line 204-212: The catch handler in the refresh flow currently assumes
err has a .message and can throw if err is null/undefined; change the block to
safely extract a message before calling showToast and console.error: compute a
message variable using a safe check (e.g., err instanceof Error ? err.message :
(err !== null && err !== undefined ? String(err) : 'unknown error')), use that
message in console.error and showToast, and then proceed to call
inFlight.dismiss(), btn.removeAttribute('disabled'), and restore btn.textContent
to originalText; update references in this handler (err, showToast,
inFlight.dismiss, btn, originalText) only.
---
Nitpick comments:
In `@frontend/src/freshness.ts`:
- Around line 186-188: The code unnecessarily uses setAttribute/removeAttribute
and a dead null-coalescing fallback on textContent: since btn is an
HTMLButtonElement, replace btn.setAttribute('disabled','true') /
btn.removeAttribute('disabled') with btn.disabled = true / btn.disabled = false,
remove the redundant "?? 'Refresh'" fallback when reading btn.textContent, and
update the corresponding test assertion (which currently checks
getAttribute('disabled') === 'true') to assert btn.disabled === true; target the
btn variable usage in freshness.ts and the test that asserts disabled.
🪄 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: b388fedb-d119-4daf-8c3e-1be85f0185e0
📒 Files selected for processing (2)
frontend/src/__tests__/freshness.test.tsfrontend/src/freshness.ts
| } catch (err) { | ||
| console.error('Refresh failed:', err); | ||
| inFlight.dismiss(); | ||
| showToast({ | ||
| message: `Refresh failed: ${(err as Error).message ?? 'unknown error'}`, | ||
| kind: 'error', | ||
| }); | ||
| btn.removeAttribute('disabled'); | ||
| btn.textContent = originalText; |
There was a problem hiding this comment.
Minor: guard against non-Error throws in the catch block.
(err as Error).message ?? 'unknown error' will throw a TypeError if err happens to be null or undefined (legal, if unusual, throw values), masking the real failure with a crash inside the error handler. ?? only rescues you when the expression returns undefined/null, not when the property access itself explodes.
🛡️ Safer extraction
- } catch (err) {
- console.error('Refresh failed:', err);
- inFlight.dismiss();
- showToast({
- message: `Refresh failed: ${(err as Error).message ?? 'unknown error'}`,
- kind: 'error',
- });
+ } catch (err) {
+ console.error('Refresh failed:', err);
+ inFlight.dismiss();
+ const msg = err instanceof Error ? err.message : String(err);
+ showToast({
+ message: `Refresh failed: ${msg || 'unknown error'}`,
+ kind: 'error',
+ });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@frontend/src/freshness.ts` around lines 204 - 212, The catch handler in the
refresh flow currently assumes err has a .message and can throw if err is
null/undefined; change the block to safely extract a message before calling
showToast and console.error: compute a message variable using a safe check
(e.g., err instanceof Error ? err.message : (err !== null && err !== undefined ?
String(err) : 'unknown error')), use that message in console.error and
showToast, and then proceed to call inFlight.dismiss(),
btn.removeAttribute('disabled'), and restore btn.textContent to originalText;
update references in this handler (err, showToast, inFlight.dismiss, btn,
originalText) only.
- Use idiomatic `btn.disabled = true/false` instead of setAttribute/ removeAttribute (btn is typed as HTMLButtonElement). - Drop the dead `?? 'Refresh'` fallback on `btn.textContent` — Element .textContent is never null so the branch is unreachable. - Guard the error toast against non-Error throws: `(err as Error).message` would raise a TypeError if `err` were null/undefined, masking the real failure with a crash inside the error handler itself. - Update the freshness tests to assert `btn.disabled` (boolean) instead of the attribute string.
…opdown/DB (#63) CodeRabbit flagged a key mismatch on PR #30: `SERVICE_FIELDS` in `settings.ts` uses `service: 'savingsplans'`, while `commitmentConfigs` in `commitmentOptions.ts` registered the AWS entry under `'savings-plans'` (hyphenated). Any lookup via `isValidCombination`/`getCommitmentConfig` with the non-hyphenated identifier silently fell through to `_default` instead of hitting the Savings-Plans-specific config. Today both configs are identical so behaviour is unchanged, but any future restriction added to `'savings-plans'` would have been silently skipped. CodeRabbit's suggestion was to rewrite `SERVICE_FIELDS` to the hyphenated form, but `field.service` in `SERVICE_FIELDS` flows directly to `api.updateServiceConfig(provider, service, cfg)` (settings.ts:1443) which persists `service` as the primary key in the `service_configs` table — existing production rows are `('aws', 'savingsplans')`, and renaming SERVICE_FIELDS without a DB migration would have created duplicate rows and orphaned the old config. The dropdown at `index.html:112,703` (`<option value="savingsplans">`) and the backend CLI flag (`cmd/main.go:92`) also use the non-hyphenated form. Fix by renaming the `commitmentConfigs.aws` key from `'savings-plans'` to `savingsplans` so every frontend call site — the SERVICE_FIELDS path, the plan-form dropdown path, and any direct lookups — resolves to the Savings-Plans-specific config. Update the two test cases in `commitmentOptions.test.ts` to match. This addresses the only live CodeRabbit finding across all open/closed PRs (24-39, 52-54); PR #52's nitpicks were already resolved in db429fe, and the remaining PR #30 nitpicks (double `Promise.resolve()` in settings.test.ts:854, memorydb absent from SERVICE_FIELDS) apply only to code that lives on PR #30's branch and not the base.
Verified end-to-end in browserDrove the actual production bundle ( Three snapshots over a single Refresh click:
Confirms the coderabbit nitpick fix landed in the shipped bundle: the disabled boolean attribute is set via the Tests + typecheck (already noted in the PR body) remain green. |
cristim
added
triaged
1 participant
Closes #86
Summary
/api/recommendations/refreshcall is in flight.Test plan
npx jest src/__tests__/freshness.test.ts— 17/17 pass, including two new cases covering the in-flight button state + toast sequence and the error-path rollback.npx tsc --noEmit— clean.