fix(ux/opportunities): re-query on filter change and persist to URL - #488
Conversation
The Opportunities page captured global provider/account filter changes in state but never re-queried the list — the only way to see filtered results was to navigate away and back. A hard refresh also wiped the filter back to "All" because state was seeded from defaults, not the URL. Root cause: setupRecommendationsHandlers() was a no-op left over from Bundle B's removal of the legacy per-section filter <select>s; the matching state.subscribeProvider / state.subscribeAccount wiring (now used by dashboard.ts) never landed for Opportunities. Changes: - recommendations.ts: subscribe to provider/account state changes and re-fire loadRecommendations() when the Opportunities tab is active, mirroring the dashboard pattern. Active-tab guard avoids burning an API call for a section the user isn't looking at. - topbar-filters.ts: hydrate provider + account from URL query params on init (before chip mount, so chip seeds pick up persisted state), and write back via replaceState on each chip change. Invalid provider values fall back to "" so a hand-crafted URL can't poison state. Empty selections omit the param entirely. URL is preferred over localStorage per the issue body — filter state is shareable via copy-paste link and avoids the cross-tab confusion that shared localStorage would create. Tests: - recommendations.test.ts: subscriber wiring fires loadRecommendations when opportunities-tab is active, skips when inactive, mirrors for account changes. - topbar-filters.test.ts: URL hydration of provider + account, invalid- provider sanitisation, writeback on account change, "All Accounts" removes the query param, provider change clears account in the URL. Closes #477.
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThe PR extends topbar filter functionality with URL persistence and live recommendations refresh. Filter selections (provider and account) now persist in query parameters and are restored on page load; when users change filters, the URL updates and state subscriptions trigger immediate recommendations refetch if the Opportunities tab is active. ChangesFilter Persistence and Live Refresh
Sequence Diagram(s)sequenceDiagram
participant User
participant Chip as Provider/Account Chip
participant state
participant URL as URL Query Params
participant Subscription as setupRecommendationsHandlers
participant api
User->>Chip: Change filter (on Opportunities page)
Chip->>state: Update provider/account in state
Chip->>URL: Write to query params
state->>Subscription: Trigger subscribeProvider/subscribeAccount callback
Subscription->>Subscription: Check if opportunities-tab is active
alt Tab is active
Subscription->>api: loadRecommendations()
api-->>Subscription: Return updated data
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
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)
Comment |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/recommendations.ts`:
- Around line 171-176: A single provider change can trigger both
state.subscribeProvider and state.subscribeAccount callbacks and call
loadRecommendations() twice; to fix, implement a microtask-coalescing scheduler
(e.g., scheduleLoadRecommendations) that sets a single "scheduled" flag and uses
queueMicrotask or setTimeout(...,0) to call loadRecommendations once; replace
the direct calls in the subscribeProvider and subscribeAccount callbacks with
scheduleLoadRecommendations(), and inside the scheduled task check
isOpportunitiesTabActive() before invoking loadRecommendations() and clear the
scheduled flag so subsequent changes can schedule again.
🪄 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: 0e2c4c66-0ea8-4d25-a1d6-8f63d6ef54f6
📒 Files selected for processing (4)
frontend/src/__tests__/recommendations.test.tsfrontend/src/__tests__/topbar-filters.test.tsfrontend/src/recommendations.tsfrontend/src/topbar-filters.ts
CodeRabbit pass 1 (#488). The topbar provider-change handler updates both state slots in sequence (clear accounts, then set provider — the #185 ordering rule), which fires both subscribers from one user action and triggered two back-to-back loadRecommendations() calls. Beyond the wasted API call, the two requests could race: if the first lands after the second, the user sees stale results overwriting fresh ones. Coalesce via queueMicrotask so all subscriber fires from a single sync call chain collapse into one reload. Active-tab is re-checked inside the microtask in case the user switched tabs between the change and the deferred reload. Adds a regression test that asserts the back-to-back account+provider fire pattern triggers exactly one getRecommendations() call.
|
CR pass 1 addressed:
@coderabbitai review |
|
✅ Actions performedReview 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
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.
Summary
The Opportunities page captured global provider/account filter changes in state but never re-queried the list. The only way to see filtered results was to navigate away and back. A hard refresh also wiped the filter back to "All".
Root cause:
setupRecommendationsHandlers()was a no-op left over from Bundle B's removal of the legacy per-section filter<select>s; the matchingstate.subscribeProvider/state.subscribeAccountwiring (used bydashboard.ts) never landed for Opportunities.Fix in two parts:
recommendations.tssubscribes to provider/account state changes and re-firesloadRecommendations()when the Opportunities tab is active, mirroring the dashboard pattern. Active-tab guard avoids burning an API call for a section the user isn't looking at.topbar-filters.tshydrates provider + account from?provider=/?account=on init (before chip mount, so chip seeds pick up the persisted state), and writes back viahistory.replaceStateon each chip change. Invalid provider values fall back to "" so a hand-crafted URL can't poison state. Empty selections omit the param entirely.URL preferred over localStorage per the issue body — filter state is shareable via copy-paste link and avoids the cross-tab confusion shared localStorage would create.
Test plan
recommendations.test.ts: subscriber wiring firesloadRecommendationswhenopportunities-tabis active, skips when inactive, mirrors for account changestopbar-filters.test.ts: URL hydration of provider + account, invalid-provider sanitisation, writeback on account change, "All Accounts" removes the query param, provider change clears account in the URLnpm test— 1749 passed, 1 skipped (pre-existing)npm run typecheck— cleannpm run build— clean?provider=aws&account=…and confirm filters surviveCloses #477.
Summary by CodeRabbit