Repository navigation
feat(filters): wire provider/account subscriptions to RI Exchange tab - #582
Conversation
…closes #186) The RI Exchange sub-tab was the only main tab that did not reload when the global provider/account filter changed. Dashboard, Recommendations, Plans, and History already subscribed to state.subscribeProvider / state.subscribeAccount; RI Exchange was silently skipped. Changes: - Import state module into riexchange.ts. - Add isRIExchangeSubtabActive() guard (mirrors the Recommendations tab pattern) so filter changes off-screen don't trigger unnecessary fetches. - setupRIExchangeHandlers now subscribes both state listeners; changes are coalesced via a queueMicrotask to avoid double-reloads when the topbar fires provider+account updates in sequence (same pattern as recommendations.ts). - Tests: 3 new assertions pin subscription registration, active-tab reload, and hidden-tab skip.
|
Warning Review limit reached
More reviews will be available in 18 minutes and 9 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, 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 include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThis PR addresses issue ChangesRI Exchange subscription-based filtering
🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested labels
🚥 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 |
cristim
added
priority/p2
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
frontend/src/__tests__/riexchange.test.ts (2)
477-525: ⚡ Quick winConsider adding test coverage for account subscription.
The test suite verifies provider change behavior but not account change, even though the PR adds subscriptions for both filters. Since both use the same
scheduleReloadcallback, the logic is already proven correct, but explicitly testing the account path would improve confidence and completeness.📋 Suggested test case
Add after the existing "provider change does NOT trigger..." test:
it('an account change triggers loadRIExchange when the sub-tab is active', async () => { setupRIExchangeHandlers(); _accountListeners.forEach(cb => cb()); await new Promise(r => setTimeout(r, 0)); expect(api.listConvertibleRIs).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__/riexchange.test.ts` around lines 477 - 525, Add a test that mirrors the provider-change test but exercises the account subscription: after calling setupRIExchangeHandlers() iterate over _accountListeners and invoke each callback (e.g. _accountListeners.forEach(cb => cb())), await a microtask (e.g. setTimeout 0) and assert that api.listConvertibleRIs was called; place this new it(...) immediately after the existing "provider change does NOT trigger..." test so it verifies the subscribeAccount path uses the same scheduleReload behavior.
477-525: ⚡ Quick winConsider adding test coverage for coalescing behavior.
The PR description emphasizes that the implementation "coalesces provider/account change events via queueMicrotask to prevent duplicate fetches when both change in sequence." Adding an explicit test that fires both listeners and verifies a single reload would directly validate this key requirement.
🔄 Suggested test case
Add after the account change tests:
it('coalesces provider and account changes into a single reload', async () => { setupRIExchangeHandlers(); // Simulate topbar filter cascade: provider change triggers account reset _providerListeners.forEach(cb => cb()); _accountListeners.forEach(cb => cb()); await new Promise(r => setTimeout(r, 0)); // Should be called once, not twice expect(api.listConvertibleRIs).toHaveBeenCalledTimes(1); });🤖 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__/riexchange.test.ts` around lines 477 - 525, Add a test that verifies coalescing by invoking both _providerListeners and _accountListeners in sequence and asserting only one reload call to api.listConvertibleRIs; specifically, after calling the listener callbacks (from the setup created in setupRIExchangeHandlers()), wait a microtask (e.g., await new Promise(r => setTimeout(r, 0))) and expect api.listConvertibleRIs toHaveBeenCalledTimes(1) to ensure queueMicrotask batching prevents duplicate fetches.
🤖 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__/riexchange.test.ts`:
- Around line 477-525: Add a test that mirrors the provider-change test but
exercises the account subscription: after calling setupRIExchangeHandlers()
iterate over _accountListeners and invoke each callback (e.g.
_accountListeners.forEach(cb => cb())), await a microtask (e.g. setTimeout 0)
and assert that api.listConvertibleRIs was called; place this new it(...)
immediately after the existing "provider change does NOT trigger..." test so it
verifies the subscribeAccount path uses the same scheduleReload behavior.
- Around line 477-525: Add a test that verifies coalescing by invoking both
_providerListeners and _accountListeners in sequence and asserting only one
reload call to api.listConvertibleRIs; specifically, after calling the listener
callbacks (from the setup created in setupRIExchangeHandlers()), wait a
microtask (e.g., await new Promise(r => setTimeout(r, 0))) and expect
api.listConvertibleRIs toHaveBeenCalledTimes(1) to ensure queueMicrotask
batching prevents duplicate fetches.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1d473870-8815-4646-82da-b53aa7cf16d8
📒 Files selected for processing (2)
frontend/src/__tests__/riexchange.test.tsfrontend/src/riexchange.ts
…R 4344791727) Address two CR nitpicks: verify subscribeAccount path triggers a reload when the sub-tab is active, and that queueMicrotask batches simultaneous provider+account changes into one loadRIExchange call.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
1 participant
Summary
subscribeProvider+subscribeAccountsubscriptions tosetupRIExchangeHandlerswith an active-subtab guard to skip off-screen reloadsqueueMicrotaskto prevent double-fetches when the topbar fires both a provider and account change in sequenceCloses #186
Test plan
cd frontend && npx jest riexchangepasses (35 tests including 3 new subscription tests)cd frontend && npx tsc --noEmitpassesSummary by CodeRabbit