fix(ux/home): re-query Savings-over-time chart on filter chip change - #500
fix(ux/home): re-query Savings-over-time chart on filter chip change#500cristim wants to merge 2 commits into
Conversation
The Home tab's Savings-over-time chart rendered identical data across account selections because the dashboard subscribers fired loadDashboard() on every filter change without an active-tab guard or coalescing, mirroring a gap the Opportunities page had before PR #488 (closes #477). Apply the same pattern to dashboard.ts: - Active-tab guard: only fire loadDashboard() when #home-tab is active. switchTab('home') already reloads on entry, so a filter change while the user is on Opportunities/Plans/Purchases can defer; this avoids burning /dashboard/summary, /recommendations, and /history/analytics round-trips for a section the user is not looking at. - Microtask coalescing: topbar-filters.ts updates BOTH state slots on a provider chip change (clear accounts then set provider, per the #185 ordering rule), firing both subscribers from one user action. Without coalescing this would trigger two loadDashboard() calls back-to-back plus a stale-overwrite risk if responses race. queueMicrotask runs once after the synchronous state-mutation chain settles. - Re-check the active-tab flag inside the microtask so a tab-switch between chip click and microtask flush skips the now-unneeded fetch. URL persistence is already global via topbar-filters.ts (PR #488); Home inherits it for free. Test coverage (dashboard.test.ts, new describe 'subscriber wiring (#498)'): - Wiring assertion: setupDashboardHandlers registers callbacks with state.subscribeProvider and state.subscribeAccount. - Account chip change re-queries getSavingsAnalytics when home-tab is active. - Provider chip change re-queries getSavingsAnalytics when home-tab is active. - Inactive-tab guard: no fetch when home-tab is not .active. - Coalescing: back-to-back provider+account fires trigger one reload. - Bug-was-fixed: two consecutive account changes produce two fetches with distinct account_ids ([A] then [B]), proving the chart re-queries with fresh data rather than the same payload. Closes #498.
|
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)
📝 WalkthroughWalkthroughThis PR fixes Issue ChangesHome tab active gating and subscriber coalescing
Sequence DiagramsequenceDiagram
participant StateProvider as State Provider
participant Handler as setupDashboardHandlers
participant ActiveCheck as isHomeTabActive()
participant Microtask as queueMicrotask
participant Dashboard as loadDashboard()
participant API as api.getSavingsAnalytics
StateProvider->>Handler: provider/account change notification
Handler->>ActiveCheck: check Home tab active
ActiveCheck-->>Handler: boolean result
alt Home tab is active
Handler->>Microtask: scheduleReload (queueMicrotask)
StateProvider->>Handler: additional change (same microtask)
Handler-->>Microtask: reloadQueued true → skip duplicate
Microtask->>Dashboard: flush → call loadDashboard()
Dashboard->>API: getSavingsAnalytics(account_ids)
API-->>Dashboard: chart data
else Home tab not active
Handler-->>Handler: bail out, no scheduling
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related issues
Possibly related PRs
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 docstrings
🧪 Generate unit tests (beta)
Comment |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
1 similar comment
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
🧹 Nitpick comments (1)
frontend/src/__tests__/dashboard.test.ts (1)
784-903: ⚡ Quick winAdd one regression test for “active at fire-time, inactive at microtask flush.”
Current tests validate active/inactive at callback time, but not the mid-flight tab switch that Line 77–81 specifically guards. Adding that case would lock in the cancellation behavior.
Suggested test addition
+ test('cancels queued reload when Home becomes inactive before microtask flush', async () => { + const tab = document.getElementById('home-tab')!; + tab.classList.add('active'); + + setupDashboardHandlers(); + const accountCb = (state.subscribeAccount as jest.Mock).mock.calls[0]?.[0] as () => void; + + (api.getDashboardSummary as jest.Mock).mockClear(); + (api.getSavingsAnalytics as jest.Mock).mockClear(); + + accountCb(); // queued while active + tab.classList.remove('active'); // user switches tabs before flush + + await new Promise((r) => setTimeout(r, 0)); + await new Promise((r) => setTimeout(r, 0)); + + expect(api.getDashboardSummary).not.toHaveBeenCalled(); + expect(api.getSavingsAnalytics).not.toHaveBeenCalled(); + });🤖 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__/dashboard.test.ts` around lines 784 - 903, Add a regression test that exercises the “active at fire-time, inactive at microtask flush” path: write a new test using setupDashboardHandlers() to grab the subscriber callback (state.subscribeAccount or state.subscribeProvider), ensure the `#home-tab` has .active, call the subscriber (e.g., accountCb()), then synchronously remove the .active class from the `#home-tab` before yielding to the microtask/macrotask flush (the two await new Promise(r => setTimeout(r,0)) calls used elsewhere), and finally assert that api.getSavingsAnalytics and api.getDashboardSummary were NOT called; place the test alongside the other dashboard tests and follow the same mockClear/await pattern so it verifies the mid-flight tab-switch cancellation guarded by setupDashboardHandlers.
🤖 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/__tests__/dashboard.test.ts`:
- Around line 784-903: Add a regression test that exercises the “active at
fire-time, inactive at microtask flush” path: write a new test using
setupDashboardHandlers() to grab the subscriber callback (state.subscribeAccount
or state.subscribeProvider), ensure the `#home-tab` has .active, call the
subscriber (e.g., accountCb()), then synchronously remove the .active class from
the `#home-tab` before yielding to the microtask/macrotask flush (the two await
new Promise(r => setTimeout(r,0)) calls used elsewhere), and finally assert that
api.getSavingsAnalytics and api.getDashboardSummary were NOT called; place the
test alongside the other dashboard tests and follow the same mockClear/await
pattern so it verifies the mid-flight tab-switch cancellation guarded by
setupDashboardHandlers.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 706a14e8-f391-456a-a9f1-5fe88fb19662
📒 Files selected for processing (2)
frontend/src/__tests__/dashboard.test.tsfrontend/src/dashboard.ts
Address CodeRabbit nitpick on PR #500: lock in the inside-microtask active-tab re-check by exercising the path where the chip change fires while Home is active but the user switches tabs before the microtask flushes. The existing tests verified active/inactive at callback time but not the mid-flight cancellation guarded by lines 77-81 of dashboard.ts.
|
Addressed CR nitpick: added regression test covering the inside-microtask active-tab re-check (chip fires while Home active, user switches tab before microtask flushes, fetch is cancelled). 37 dashboard tests pass. @coderabbitai review |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 71855
|
| Test | What it verifies |
|---|---|
| registers callbacks | subscribeProvider and subscribeAccount each called once with a function |
| account chip / active | getDashboardSummary + getSavingsAnalytics each called once |
| provider chip / active | getSavingsAnalytics called once |
| inactive-tab guard | neither API called when .active absent |
| coalescing | back-to-back provider+account fires produce exactly one fetch each |
| in-microtask re-check (new) | getDashboardSummary/getSavingsAnalytics both suppressed when tab deactivated before microtask flushes |
| regression / distinct accounts | two sequential chip changes → two distinct getSavingsAnalytics calls with acct-A then acct-B |
The getCurrentAccountIDs mock in the regression test is set up with exactly three mockReturnValueOnce entries per account, which matches the three call sites reached per loadDashboard invocation (getDashboardSummary, getRecommendations, loadSavingsTrendChart). That accounting is correct.
The double await new Promise(r => setTimeout(r, 0)) flush pattern correctly drains the microtask queue and then the awaited Promise.allSettled chain — no concern there.
Summary: the implementation is solid and the test coverage is thorough. The only actionable item is the unsubscribe-return-value pattern; everything else is either correct or a pre-existing upstream concern. Happy to approve once you've considered the unsubscribe point (or confirmed single-initialization is guaranteed by the call site).
|
The nitpick is addressed: commit 8ae19d3 adds a regression test that exercises exactly the mid-flight tab-switch cancellation path (active at callback time, inactive before microtask flush). The test matches the suggested structure in the review comment. |
|
CodeRabbit review triage (single nitpick):
No actionable items outstanding. Full dashboard suite green (37 passed). Pre-merge "Docstring Coverage" warning is non-blocking and unrelated to the diff's TS test/source. @coderabbitai full review |
|
✅ Actions performedFull review triggered. |
…578) The Purchases-tab Savings History chart (modules/savings-history.ts) only wired its local period dropdown and refresh button, so changing the global Provider or Account topbar chip did not refresh it (the tab had to be left and re-entered), and it ignored the account filter entirely. Mirror the PR #500 / #488 pattern: - isPurchasesTabActive() guards the reload so a chip change while the user is on another tab defers (switchTab reloads on entry). - queueMicrotask coalesces the account-then-provider subscriber pair fired from one user action (the #185 ordering rule), collapsing two reloads into one and avoiding stale-overwrite races. - loadSavingsHistory() now forwards the selected account_id (single-select, matching the backend) and the provider chip to getSavingsAnalytics. The provider param is honoured by the backend once #502 lands; until then account_ids does the filtering. Adds regression tests covering subscriber wiring, the active-tab guard, coalescing, account_id passthrough, and two consecutive account changes producing distinct fetches. Updates app.test.ts's state mock to expose the new subscribe getters since it exercises the real savings-history module. Closes #503
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
Address CodeRabbit nitpick on PR #500: lock in the inside-microtask active-tab re-check by exercising the path where the chip change fires while Home is active but the user switches tabs before the microtask flushes. The existing tests verified active/inactive at callback time but not the mid-flight cancellation guarded by lines 77-81 of dashboard.ts.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
Summary
The Home tab's "Savings over time" chart rendered identical data across
account selections. The dashboard subscribers fired
loadDashboard()onevery filter chip change but lacked the active-tab guard + microtask
coalescing that PR #488 (closes #477) added to Opportunities, so the
provider-change ordering rule from #185 (clear accounts, then set
provider) caused back-to-back redundant reloads, and tab-switching could
produce stale renders.
Fix mirrors PR #488's pattern in
dashboard.ts:loadDashboard()only runs when#home-tabis.active.switchTab('home')reloads on entry, so a filter changewhile the user is on another tab can defer (avoids burning
/dashboard/summary,/recommendations,/history/analyticsround-trips for an unseen section).
queueMicrotaskcollapses theaccount-then-provider subscriber pair (fired from one user action via
the fix(dashboard): account dropdown change doesn't reliably refresh dashboard data #185 ordering rule in
topbar-filters.ts) into one reload. Avoidsstale-overwrite races if responses arrive out of order.
chip click and microtask flush cancels the now-unneeded fetch.
URL persistence for provider/account is already global via
topbar-filters.ts(added in PR #488), so Home inherits it for free.Test plan
New
describe('subscriber wiring (issue #498)')indashboard.test.ts:setupDashboardHandlersregisters callbacks withstate.subscribeProviderandstate.subscribeAccount.getSavingsAnalyticswhen#home-tabis active.getSavingsAnalyticswhen#home-tabis active.#home-tabis not.active.one reload.
fetches with distinct
account_ids(['acct-A']then['acct-B']),proving the chart re-queries with fresh data rather than re-rendering
the same payload.
npx tsc --noEmitclean.webpack --mode productionclean.pre-existing unrelated failure in
settings-purchasing-grace.test.ts(touches no files in this PR; failing on
feat/multicloud-web-frontendbefore this change).
confirm the Savings-over-time chart re-fetches with new data.
Out of scope (follow-up issues to be filed)
/history/analyticsbackend handler ignoresproviderquery param.The frontend currently doesn't pass
providertogetSavingsAnalyticsbecause the backend doesn't honour it (Athenalimitation). Switching provider chip clears accounts (per fix(dashboard): account dropdown change doesn't reliably refresh dashboard data #185), which
effectively resets the chart to "all accounts", so the chart shape
changes but isn't truly provider-filtered. Backend feature, separate
ticket.
modules/savings-history.ts(Purchases tab'ssavings-history-chart)has the same subscriber-wiring gap and additionally ignores account
selection entirely. Separate Purchases-tab ticket.
Closes #498.
Summary by CodeRabbit
Bug Fixes
Performance
Tests