feat: add polling to BackendCapabilitiesBanner, persist OnboardingChe… - #1610
Conversation
…cklist, add modal to CorrelationHeatmap, add breakdown tooltip to DriftGaugeGrid
|
Someone is attempting to deploy a commit to the ritik4ever's projects Team on Vercel. A member of the Team first needs to authorize it. |
|
@madhavi-0701 Great news! 🎉 Based on an automated assessment of this PR, the linked Wave issue(s) no longer count against your application limits. You can now already apply to more issues while waiting for a review of this PR. Keep up the great work! 🚀 |
📝 WalkthroughWalkthroughFrontend updates add readiness polling and banner stabilization, correlation heatmap drill-downs, per-asset drift breakdowns, and local-storage persistence with reset support for onboarding progress. Tests were updated for the new query context, interactions, persistence, and polling behavior. ChangesBackend readiness banner
Correlation heatmap drill-down
Drift breakdown
Onboarding progress persistence
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant CorrelationHeatmap
participant usePriceCandlestick
participant Modal
User->>CorrelationHeatmap: click distinct asset pair cell
CorrelationHeatmap->>usePriceCandlestick: request daily close prices
usePriceCandlestick-->>CorrelationHeatmap: return price series
CorrelationHeatmap->>CorrelationHeatmap: calculate correlation trends
CorrelationHeatmap-->>Modal: show selected-pair trend details
sequenceDiagram
participant BackendCapabilitiesBanner
participant useReadinessQuery
participant CapabilitiesEndpoint
BackendCapabilitiesBanner->>useReadinessQuery: subscribe to readiness state
useReadinessQuery->>CapabilitiesEndpoint: poll using visibility-aware interval
CapabilitiesEndpoint-->>useReadinessQuery: notices and errors
useReadinessQuery-->>BackendCapabilitiesBanner: updated readiness result
BackendCapabilitiesBanner->>BackendCapabilitiesBanner: update only when notice content changes
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
🧹 Nitpick comments (3)
frontend/src/components/CorrelationHeatmap.test.tsx (1)
27-35: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winShared
QueryClientleaks cache between tests.A single module-level client keeps candle query cache and refetch timers alive across all tests in the file, so the drill-down test's results can bleed into later tests. Create the client per test, or at least clear it in
afterEach.♻️ Minimal change
-afterEach(cleanup) +afterEach(() => { + cleanup() + queryClient.clear() +})🤖 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/components/CorrelationHeatmap.test.tsx` around lines 27 - 35, Update the test setup around the module-level queryClient and Wrapper so each test receives a fresh QueryClient, or clear the shared client during afterEach alongside cleanup. Ensure cached query data and refetch timers cannot persist between tests.frontend/src/components/CorrelationHeatmap.tsx (1)
271-309: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the repeated trend row.
The three rows differ only by label and value while duplicating the same pair of threshold ternaries six times. A small local row component (or a
thresholdStyle(value)helper) removes the copy/paste and keeps thresholds in one place.♻️ Sketch
const TREND_ROWS = [ { label: 'Full period', key: 'fullPeriod' }, { label: 'Recent half', key: 'recent' }, { label: 'Older half', key: 'older' }, ] as const function trendStyle(value: number) { if (value > 0.3) return { color: '`#16a34a`', backgroundColor: '`#dcfce7`' } if (value < -0.3) return { color: '`#dc2626`', backgroundColor: '`#fee2e2`' } return { color: '`#6b7280`', backgroundColor: '`#f3f4f6`' } }<div className="space-y-3"> {TREND_ROWS.map(({ label, key }) => ( <div key={key} className="flex items-center justify-between p-3 rounded-lg bg-gray-50 dark:bg-gray-700/50"> <span className="text-sm text-gray-600 dark:text-gray-400">{label}</span> <span className="text-sm font-semibold px-2 py-0.5 rounded" style={trendStyle(trendCorrelations[key])}> {trendCorrelations[key].toFixed(3)} </span> </div> ))} </div>🤖 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/components/CorrelationHeatmap.tsx` around lines 271 - 309, Extract the duplicated trend rows in the trendCorrelations rendering into a shared TREND_ROWS definition and mapped row markup, using a single trendStyle(value) helper for the positive, negative, and neutral threshold colors. Preserve the existing labels, keys, formatting, layout classes, and threshold behavior.frontend/src/components/BackendCapabilitiesBanner.tsx (1)
50-58: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winThis ref does not suppress renders and is mutated during render.
When the readiness report changes but produces equivalent notices,
BackendCapabilitiesBannerstill re-renders because the hook result changed; updatinglastNotices.currentcannot prevent that. It also reads and writesref.currentduring render, which React documents as unpredictable outside initialization. (uk.react.dev)Move notice stabilization into
useReadinessQueryusing a selected, structurally shared notice payload, then render that value directly instead of maintaining a render-phase ref.🤖 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/components/BackendCapabilitiesBanner.tsx` around lines 50 - 58, Move notice stabilization out of BackendCapabilitiesBanner and into useReadinessQuery by selecting the notices payload with structural sharing, so equivalent readiness updates reuse the previous notices reference. Remove the lastNotices ref and render-phase comparison, and have BackendCapabilitiesBanner consume the stabilized notices directly while preserving the existing noticesEqual equivalence behavior.Source: MCP tools
🤖 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/App.tsx`:
- Line 310: Unify readiness state in App by deriving showBackendBanner,
contentTopPad, and errorTop from the same useReadinessQuery result consumed by
BackendCapabilitiesBanner. Update the banner integration so both layout and
banner decisions use one readiness source, eliminating the separate
useReadinessReport polling path.
In `@frontend/src/components/BackendCapabilitiesBanner.test.tsx`:
- Around line 117-131: Update the test around BackendCapabilitiesBanner so it
keeps the same mounted component while simulating the poll change: retain the
render result, change mockQuery’s return value, and call rerender instead of
unmounting and rendering again. Preserve the assertions for the initial and
updated notices to verify an existing banner responds to changed notices.
In `@frontend/src/components/CorrelationHeatmap.test.tsx`:
- Around line 84-91: Update the drill-down test around CorrelationHeatmap to
mock usePriceCandlestick, or preload the relevant pair queries, so the trend
rendering and fetch behavior are deterministic. After clicking
correlation-cell-0-1, assert that the modal body contains both XLM and BTC in
addition to the existing title assertions, verifying the clicked asset pair
context.
In `@frontend/src/components/CorrelationHeatmap.tsx`:
- Around line 57-60: Update the return calculations in the loop within
CorrelationHeatmap so each zero/missing-value guard checks the previous price
used as the divisor (`pricesA[i - 1]` and `pricesB[i - 1]`), rather than the
current price at index i. Preserve the existing fallback of 0 while ensuring
denominators are validated before division.
- Around line 100-116: Update the trendCorrelations useMemo guard to return null
unless each candle dataset’s asset matches the currently selectedPair,
preventing placeholder candles from the previous pair from being used. Include
selectedPair in the memo dependency array while preserving the existing
correlation calculations for matching assets.
- Around line 310-312: Update the correlation trend rendering in
CorrelationHeatmap to use the isLoading and isError states from both
usePriceCandlestick calls, and handle datasets with fewer than 7 candles
explicitly. Show the loading placeholder only while either request is loading,
and render a clear failure or insufficient-history state instead of leaving the
loading message indefinitely.
In `@frontend/src/components/DriftGauge.test.tsx`:
- Around line 83-110: Update the test for DriftGaugeGrid’s title-hover tooltip
to assert the calculated contribution values in addition to ordering: verify XLM
displays a 42% contribution and the breakdown shows a 12.0% total, while
preserving the existing asset-order and drift assertions.
In `@frontend/src/components/DriftGauge.tsx`:
- Around line 254-265: Update the DriftGauge component to generate a stable
tooltip identifier with React useId, assign it to the conditional breakdown
element with role="tooltip", and add the matching aria-describedby reference to
the drift-gauge-heading element. Keep the association valid when showBreakdown
is false.
- Around line 230-240: Move the assets-empty check in DriftGaugeGrid below the
sortedAssets and totalDrift useMemo hooks so every render invokes hooks in the
same order. Preserve returning null for empty assets while keeping the existing
memoized calculations unchanged.
In `@frontend/src/components/OnboardingChecklist.tsx`:
- Around line 100-170: Update the liveStepStatuses synchronization in
OnboardingChecklist so persisted steps are added only when a live status
transitions from false to true, preventing unrelated status changes from
resurrecting steps cleared by Reset. Consolidate the initial-load merge and
ongoing sync into one effect, track previous live statuses, and include all
referenced state in dependencies without relying on a stale completedSteps
closure. Add a regression test covering reset followed by an unrelated
live-status change while another condition remains true.
In `@frontend/src/hooks/queries/useReadinessQuery.ts`:
- Around line 57-69: The useReadinessQuery polling interval does not reliably
adapt to tab visibility changes. Add SSR-safe React visibility state updated by
a visibilitychange listener, use that isVisible state in refetchInterval to
select POLL_MS or HIDDEN_POLL_MS, and enable refetchIntervalInBackground so
hidden-tab polling continues at the slower rate.
---
Nitpick comments:
In `@frontend/src/components/BackendCapabilitiesBanner.tsx`:
- Around line 50-58: Move notice stabilization out of BackendCapabilitiesBanner
and into useReadinessQuery by selecting the notices payload with structural
sharing, so equivalent readiness updates reuse the previous notices reference.
Remove the lastNotices ref and render-phase comparison, and have
BackendCapabilitiesBanner consume the stabilized notices directly while
preserving the existing noticesEqual equivalence behavior.
In `@frontend/src/components/CorrelationHeatmap.test.tsx`:
- Around line 27-35: Update the test setup around the module-level queryClient
and Wrapper so each test receives a fresh QueryClient, or clear the shared
client during afterEach alongside cleanup. Ensure cached query data and refetch
timers cannot persist between tests.
In `@frontend/src/components/CorrelationHeatmap.tsx`:
- Around line 271-309: Extract the duplicated trend rows in the
trendCorrelations rendering into a shared TREND_ROWS definition and mapped row
markup, using a single trendStyle(value) helper for the positive, negative, and
neutral threshold colors. Preserve the existing labels, keys, formatting, layout
classes, and threshold behavior.
🪄 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 Plus
Run ID: 6d55b7ac-0bb8-4c91-9163-d127d4c1703a
⛔ Files ignored due to path filters (1)
frontend/src/components/__snapshots__/DriftGauge.test.tsx.snapis excluded by!**/*.snap
📒 Files selected for processing (11)
frontend/src/App.tsxfrontend/src/components/BackendCapabilitiesBanner.test.tsxfrontend/src/components/BackendCapabilitiesBanner.tsxfrontend/src/components/CorrelationHeatmap.test.tsxfrontend/src/components/CorrelationHeatmap.tsxfrontend/src/components/DriftGauge.test.tsxfrontend/src/components/DriftGauge.tsxfrontend/src/components/OnboardingChecklist.test.tsxfrontend/src/components/OnboardingChecklist.tsxfrontend/src/components/ui/Modal.tsxfrontend/src/hooks/queries/useReadinessQuery.ts
💤 Files with no reviewable changes (1)
- frontend/src/components/ui/Modal.tsx
| loading={readinessLoading} | ||
| belowRealtimeBar={false} | ||
| /> | ||
| <BackendCapabilitiesBanner belowRealtimeBar={false} /> |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C 6 'useReadinessReport|useReadinessQuery|readinessKeys|fetchReadinessReport|setInterval' frontend/srcRepository: ritik4ever/stellar-portfolio-rebalancer
Length of output: 32422
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== App readiness/layout declarations and banner invocation =="
sed -n '45,68p' frontend/src/App.tsx
echo
sed -n '280,325p' frontend/src/App.tsx
echo
echo "== read-only hook/body evidence =="
sed -n '1,90p' frontend/src/hooks/queries/useReadinessQuery.ts
echo
sed -n '165,208p' frontend/src/hooks/useReadinessReport.ts
echo
echo "== Behavioral model from source text =="
python3 - <<'PY'
from pathlib import Path
app = Path('frontend/src/App.tsx').read_text()
banner = Path('frontend/src/components/BackendCapabilitiesBanner.tsx').read_text()
report = Path('frontend/src/hooks/useReadinessReport.ts').read_text()
query = Path('frontend/src/hooks/queries/useReadinessQuery.ts').read_text()
checks = {
'App uses both readiness hooks': 'useReadinessReport' in app and 'useReadinessQuery' in banner and app.count('BackendCapabilitiesBanner') >= 1,
'useReadinessQuery uses React Query interval/backoff': 'useQuery' in query and 'refetchInterval: () => document.visibilityState' in query,
'useReadinessReport uses manual polling interval/backoff': 'window.setInterval' in report and 'visibilitychange' in report,
'App derives showBackendBanner from useReadinessReport': 'showBackendBanner' in app and 'notices.some' in app,
'layout top-padding class from showBackendBanner': 'pt-4' in app and 'contentTopPad' in app,
'errorTop uses report from useReadinessReport': 'errorTop' in app and 'report' in app and 'useReadinessReport' in app,
}
for name, ok in checks.items():
print(f'{name}: {ok}')
PYRepository: ritik4ever/stellar-portfolio-rebalancer
Length of output: 8725
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== App relevant sections =="
sed -n '62,80p' frontend/src/App.tsx
sed -n '200,320p' frontend/src/App.tsx
echo
echo "== useReadinessReport refresh and any App/other use/useQueryClient references =="
rg -n -C 4 'function useReadinessReport|refresh|useQueryClient|invalidateQueries|reset|useReadinessReport\(|useReadinessQuery\(' frontend/src/App.tsx frontend/src/hooks/utils frontend/src/hooks/queries frontend/src/hooks useRepository: ritik4ever/stellar-portfolio-rebalancer
Length of output: 39398
Use one readiness source for App layout and the banner.
App still derives showBackendBanner, contentTopPad, and errorTop from useReadinessReport, while BackendCapabilitiesBanner reads useReadinessQuery. Since refresh() is not passed into App, readiness can update in the banner before the page offset/error positioning catch up, and the two hooks also poll separately. Derive all readiness decisions from useReadinessQuery or pass the same hook result into the banner.
🤖 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/App.tsx` at line 310, Unify readiness state in App by deriving
showBackendBanner, contentTopPad, and errorTop from the same useReadinessQuery
result consumed by BackendCapabilitiesBanner. Update the banner integration so
both layout and banner decisions use one readiness source, eliminating the
separate useReadinessReport polling path.
| it('updates banner after a simulated capability change on subsequent poll', () => { | ||
| mockReturn({ notices: [notice('database', 'limited', 'Initial issue.')] }) | ||
| const { unmount } = render(<BackendCapabilitiesBanner />) | ||
| expect(screen.getByText(/Initial issue\./)).toBeInTheDocument() | ||
| unmount() | ||
|
|
||
| mockQuery.mockReturnValue({ | ||
| notices: [notice('database', 'limited', 'Updated issue.')], | ||
| loadError: false, | ||
| loading: false, | ||
| report: null, | ||
| refresh: vi.fn(), | ||
| }) | ||
| render(<BackendCapabilitiesBanner />) | ||
| expect(screen.getByText(/Updated issue\./)).toBeInTheDocument() |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Keep the component mounted when simulating a poll update.
Unmounting before changing the mock resets lastNotices, so this test only proves that a fresh mount renders the new notice. It would pass even if an existing banner ignored updated notices.
Update the mock return value and call rerender without unmounting.
Proposed test adjustment
- const { unmount } = render(<BackendCapabilitiesBanner />)
+ const view = render(<BackendCapabilitiesBanner />)
expect(screen.getByText(/Initial issue\./)).toBeInTheDocument()
- unmount()
mockQuery.mockReturnValue({
notices: [notice('database', 'limited', 'Updated issue.')],
loadError: false,
loading: false,
report: null,
refresh: vi.fn(),
})
- render(<BackendCapabilitiesBanner />)
+ view.rerender(<BackendCapabilitiesBanner />)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| it('updates banner after a simulated capability change on subsequent poll', () => { | |
| mockReturn({ notices: [notice('database', 'limited', 'Initial issue.')] }) | |
| const { unmount } = render(<BackendCapabilitiesBanner />) | |
| expect(screen.getByText(/Initial issue\./)).toBeInTheDocument() | |
| unmount() | |
| mockQuery.mockReturnValue({ | |
| notices: [notice('database', 'limited', 'Updated issue.')], | |
| loadError: false, | |
| loading: false, | |
| report: null, | |
| refresh: vi.fn(), | |
| }) | |
| render(<BackendCapabilitiesBanner />) | |
| expect(screen.getByText(/Updated issue\./)).toBeInTheDocument() | |
| it('updates banner after a simulated capability change on subsequent poll', () => { | |
| mockReturn({ notices: [notice('database', 'limited', 'Initial issue.')] }) | |
| const view = render(<BackendCapabilitiesBanner />) | |
| expect(screen.getByText(/Initial issue\./)).toBeInTheDocument() | |
| mockQuery.mockReturnValue({ | |
| notices: [notice('database', 'limited', 'Updated issue.')], | |
| loadError: false, | |
| loading: false, | |
| report: null, | |
| refresh: vi.fn(), | |
| }) | |
| view.rerender(<BackendCapabilitiesBanner />) | |
| expect(screen.getByText(/Updated issue\./)).toBeInTheDocument() |
🤖 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/components/BackendCapabilitiesBanner.test.tsx` around lines 117
- 131, Update the test around BackendCapabilitiesBanner so it keeps the same
mounted component while simulating the poll change: retain the render result,
change mockQuery’s return value, and call rerender instead of unmounting and
rendering again. Preserve the assertions for the initial and updated notices to
verify an existing banner responds to changed notices.
| it('opens a drill-down modal when a cell is clicked with correct asset pair context', async () => { | ||
| render(<CorrelationHeatmap assets={assets} correlations={matrices} />, { wrapper: Wrapper }) | ||
|
|
||
| fireEvent.click(screen.getByTestId('correlation-cell-0-1')) | ||
|
|
||
| expect(screen.getByText(/correlation detail/i)).toBeInTheDocument() | ||
| expect(screen.getByRole('heading', { name: /correlation detail/i })).toBeInTheDocument() | ||
| }) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
fd -H -t f 'vitest.config.*|vite.config.*' frontend --exec cat -n
fd -H -t f 'setup*.ts*' frontend/src --exec cat -n
rg -n --type=ts --type=tsx 'usePriceCandlestickQuery|PRICE_CHART' frontend/src -g '!**/*.test.*'Repository: ritik4ever/stellar-portfolio-rebalancer
Length of output: 50396
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== locate heatmap tests and component =="
fd -H -t f 'CorrelationHeatmap' frontend
echo
echo "== test excerpt =="
file="$(fd -H -t f 'CorrelationHeatmap.test.tsx' frontend | head -n1 || true)"
if [ -n "$file" ]; then
wc -l "$file"
sed -n '1,140p' "$file" | cat -n
fi
echo
echo "== component outline relevant file =="
if [ -n "$file" ]; then
comp="$(git rev-parse --show-toplevel)/frontend/src/components/CorrelationHeatmap.tsx"
if [ -f "$comp" ]; then
wc -l "$comp"
ast-grep outline "$comp" --view expanded || true
sed -n '1,260p' "$comp" | cat -n
fi
fi
echo
echo "== hook usages =="
rg -n --type=ts --type=tsx 'usePriceCandlestick|QueryClient|refetchInterval|DataChart|data:' frontend/src -g '!**/*.test.*' --glob '!**/node_modules/**' --max-count 200Repository: ritik4ever/stellar-portfolio-rebalancer
Length of output: 17764
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== remaining component render/modal section =="
file="$(git rev-parse --show-toplevel)/frontend/src/components/CorrelationHeatmap.tsx"
sed -n '260,330p' "$file" | cat -n
echo
echo "== hook file =="
hook="$(git rev-parse --show-toplevel)/frontend/src/hooks/queries/usePriceCandlestickQuery.ts" || true
if [ -f "$hook" ]; then
wc -l "$hook"
sed -n '1,220p' "$hook" | cat -n
else
fd -H -t f 'usePriceCandlestickQuery' frontend
fi
echo
echo "== global fetch/api mocks in Vitest setup files =="
sed -n '1,180p' frontend/src/test/setup.ts | cat -n
rg -n --glob '!**/node_modules/**' 'vi\.stubGlobal\(|fetch|global.fetch|axios|XMLHttpRequest|msw|setupServer|http|api|usePriceCandlestickQuery|queryClient' frontend/src frontend/vitest.config.ts frontend/src/test/setup.tsRepository: ritik4ever/stellar-portfolio-rebalancer
Length of output: 50395
Assert the clicked pair and mock the candlestick query.
The assertions only check the static modal title, so this would pass for the wrong XLM/BTC modal; assert that XLM and BTC appear in the modal body, and mock usePriceCandlestick (or preload the pair’s queries) so the trend path and its fetch are deterministic.
🤖 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/components/CorrelationHeatmap.test.tsx` around lines 84 - 91,
Update the drill-down test around CorrelationHeatmap to mock
usePriceCandlestick, or preload the relevant pair queries, so the trend
rendering and fetch behavior are deterministic. After clicking
correlation-cell-0-1, assert that the modal body contains both XLM and BTC in
addition to the existing title assertions, verifying the clicked asset pair
context.
| for (let i = 1; i < pricesA.length; i++) { | ||
| returnsA.push(pricesA[i] > 0 ? (pricesA[i] - pricesA[i - 1]) / pricesA[i - 1] : 0) | ||
| returnsB.push(pricesB[i] > 0 ? (pricesB[i] - pricesB[i - 1]) / pricesB[i - 1] : 0) | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Zero-guard checks the wrong index.
The divisor is pricesA[i - 1], but the guard inspects pricesA[i]. With a zero/missing previous close the division yields Infinity, which poisons the means and variances so the whole series collapses to 0 (only saved from NaN by clampCorrelation). Guard the denominator instead.
🐛 Proposed fix
- for (let i = 1; i < pricesA.length; i++) {
- returnsA.push(pricesA[i] > 0 ? (pricesA[i] - pricesA[i - 1]) / pricesA[i - 1] : 0)
- returnsB.push(pricesB[i] > 0 ? (pricesB[i] - pricesB[i - 1]) / pricesB[i - 1] : 0)
- }
+ for (let i = 1; i < pricesA.length; i++) {
+ if (!(pricesA[i - 1] > 0) || !(pricesB[i - 1] > 0)) continue
+ returnsA.push((pricesA[i] - pricesA[i - 1]) / pricesA[i - 1])
+ returnsB.push((pricesB[i] - pricesB[i - 1]) / pricesB[i - 1])
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| for (let i = 1; i < pricesA.length; i++) { | |
| returnsA.push(pricesA[i] > 0 ? (pricesA[i] - pricesA[i - 1]) / pricesA[i - 1] : 0) | |
| returnsB.push(pricesB[i] > 0 ? (pricesB[i] - pricesB[i - 1]) / pricesB[i - 1] : 0) | |
| } | |
| for (let i = 1; i < pricesA.length; i++) { | |
| if (!(pricesA[i - 1] > 0) || !(pricesB[i - 1] > 0)) continue | |
| returnsA.push((pricesA[i] - pricesA[i - 1]) / pricesA[i - 1]) | |
| returnsB.push((pricesB[i] - pricesB[i - 1]) / pricesB[i - 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/components/CorrelationHeatmap.tsx` around lines 57 - 60, Update
the return calculations in the loop within CorrelationHeatmap so each
zero/missing-value guard checks the previous price used as the divisor
(`pricesA[i - 1]` and `pricesB[i - 1]`), rather than the current price at index
i. Preserve the existing fallback of 0 while ensuring denominators are validated
before division.
| const trendCorrelations = useMemo(() => { | ||
| if (!candleDataA || !candleDataB) return null | ||
| const pricesA = candleDataA.candles.map((c) => c.close) | ||
| const pricesB = candleDataB.candles.map((c) => c.close) | ||
| if (pricesA.length < 7 || pricesB.length < 7) return null | ||
|
|
||
| const minLen = Math.min(pricesA.length, pricesB.length) | ||
| const alignedA = pricesA.slice(pricesA.length - minLen) | ||
| const alignedB = pricesB.slice(pricesB.length - minLen) | ||
|
|
||
| const fullPeriod = computePearsonCorrelation(alignedA, alignedB) | ||
| const mid = Math.floor(minLen / 2) | ||
| const recent = computePearsonCorrelation(alignedA.slice(mid), alignedB.slice(mid)) | ||
| const older = computePearsonCorrelation(alignedA.slice(0, mid), alignedB.slice(0, mid)) | ||
|
|
||
| return { fullPeriod, recent, older } | ||
| }, [candleDataA, candleDataB]) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Guard against stale candle data from the previously selected pair.
usePriceCandlestick uses placeholderData: (prev) => prev, so right after switching pairs the hooks can return the previous asset's candles while the new query resolves. Since the memo only depends on the data objects, the modal will render numbers computed for the old pair under the new pair's heading. Validate the returned asset against selectedPair (and include it in the deps).
🛡️ Proposed guard
const trendCorrelations = useMemo(() => {
- if (!candleDataA || !candleDataB) return null
+ if (!selectedPair || !candleDataA || !candleDataB) return null
+ if (
+ candleDataA.asset !== selectedPair.rowAsset ||
+ candleDataB.asset !== selectedPair.columnAsset
+ ) {
+ return null
+ }
const pricesA = candleDataA.candles.map((c) => c.close)
@@
return { fullPeriod, recent, older }
- }, [candleDataA, candleDataB])
+ }, [candleDataA, candleDataB, selectedPair])📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const trendCorrelations = useMemo(() => { | |
| if (!candleDataA || !candleDataB) return null | |
| const pricesA = candleDataA.candles.map((c) => c.close) | |
| const pricesB = candleDataB.candles.map((c) => c.close) | |
| if (pricesA.length < 7 || pricesB.length < 7) return null | |
| const minLen = Math.min(pricesA.length, pricesB.length) | |
| const alignedA = pricesA.slice(pricesA.length - minLen) | |
| const alignedB = pricesB.slice(pricesB.length - minLen) | |
| const fullPeriod = computePearsonCorrelation(alignedA, alignedB) | |
| const mid = Math.floor(minLen / 2) | |
| const recent = computePearsonCorrelation(alignedA.slice(mid), alignedB.slice(mid)) | |
| const older = computePearsonCorrelation(alignedA.slice(0, mid), alignedB.slice(0, mid)) | |
| return { fullPeriod, recent, older } | |
| }, [candleDataA, candleDataB]) | |
| const trendCorrelations = useMemo(() => { | |
| if (!selectedPair || !candleDataA || !candleDataB) return null | |
| if ( | |
| candleDataA.asset !== selectedPair.rowAsset || | |
| candleDataB.asset !== selectedPair.columnAsset | |
| ) { | |
| return null | |
| } | |
| const pricesA = candleDataA.candles.map((c) => c.close) | |
| const pricesB = candleDataB.candles.map((c) => c.close) | |
| if (pricesA.length < 7 || pricesB.length < 7) return null | |
| const minLen = Math.min(pricesA.length, pricesB.length) | |
| const alignedA = pricesA.slice(pricesA.length - minLen) | |
| const alignedB = pricesB.slice(pricesB.length - minLen) | |
| const fullPeriod = computePearsonCorrelation(alignedA, alignedB) | |
| const mid = Math.floor(minLen / 2) | |
| const recent = computePearsonCorrelation(alignedA.slice(mid), alignedB.slice(mid)) | |
| const older = computePearsonCorrelation(alignedA.slice(0, mid), alignedB.slice(0, mid)) | |
| return { fullPeriod, recent, older } | |
| }, [candleDataA, candleDataB, selectedPair]) |
🤖 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/components/CorrelationHeatmap.tsx` around lines 100 - 116,
Update the trendCorrelations useMemo guard to return null unless each candle
dataset’s asset matches the currently selectedPair, preventing placeholder
candles from the previous pair from being used. Include selectedPair in the memo
dependency array while preserving the existing correlation calculations for
matching assets.
| it('shows per-asset breakdown tooltip on title hover sorted by largest drift first', () => { | ||
| render(<DriftGaugeGrid assets={sampleAssets} />) | ||
|
|
||
| const heading = screen.getByText('Allocation Drift') | ||
| fireEvent.mouseEnter(heading) | ||
|
|
||
| const tooltip = screen.getByRole('tooltip') | ||
| expect(tooltip).toHaveTextContent('Drift Breakdown') | ||
|
|
||
| const tooltipText = tooltip.textContent || '' | ||
| const xlmIndex = tooltipText.indexOf('XLM') | ||
| const btcIndex = tooltipText.indexOf('BTC') | ||
| const ethIndex = tooltipText.indexOf('ETH') | ||
| const usdcIndex = tooltipText.indexOf('USDC') | ||
|
|
||
| expect(xlmIndex).toBeGreaterThan(-1) | ||
| expect(btcIndex).toBeGreaterThan(-1) | ||
| expect(ethIndex).toBeGreaterThan(-1) | ||
| expect(usdcIndex).toBeGreaterThan(-1) | ||
|
|
||
| expect(xlmIndex).toBeLessThan(btcIndex) | ||
| expect(btcIndex).toBeLessThan(usdcIndex) | ||
| expect(usdcIndex).toBeLessThan(ethIndex) | ||
|
|
||
| expect(tooltip).toHaveTextContent(/Total absolute drift/) | ||
| expect(tooltip.textContent).toContain('XLM') | ||
| expect(tooltip.textContent).toContain('+5.0%') | ||
| }) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Assert the calculated contribution percentages.
This verifies order but not the new percentage calculation. With this fixture, assert XLM’s 42% contribution and the 12.0% total so regressions in the breakdown formula fail.
🤖 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/components/DriftGauge.test.tsx` around lines 83 - 110, Update
the test for DriftGaugeGrid’s title-hover tooltip to assert the calculated
contribution values in addition to ordering: verify XLM displays a 42%
contribution and the breakdown shows a 12.0% total, while preserving the
existing asset-order and drift assertions.
| const [showBreakdown, setShowBreakdown] = useState(false) | ||
|
|
||
| if (assets.length === 0) return null | ||
|
|
||
| const sortedAssets = useMemo(() => { | ||
| return [...assets].sort((a, b) => Math.abs(b.current - b.target) - Math.abs(a.current - a.target)) | ||
| }, [assets]) | ||
|
|
||
| const totalDrift = useMemo(() => { | ||
| return assets.reduce((sum, a) => sum + Math.abs(a.current - a.target), 0) | ||
| }, [assets]) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Inspect the conditional return and all hooks in this component.
nl -ba frontend/src/components/DriftGauge.tsx | sed -n '226,242p'
# Check whether tests cover an empty-to-populated rerender transition.
rg -n -C2 'rerender|assets=\{\[\]\}|DriftGaugeGrid' frontend/src/components/DriftGauge.test.tsxRepository: ritik4ever/stellar-portfolio-rebalancer
Length of output: 1415
🏁 Script executed:
#!/bin/bash
set -eu
echo "== Component outline/matches =="
rg -n "function DriftGaugeGrid|const DriftGaugeGrid|useState|useMemo|useEffect|useRef|assets\.length === 0|showBreakdown" frontend/src/components/DriftGauge.tsx
echo
echo "== DriftGaugeGrid body =="
python3 - <<'PY'
from pathlib import Path
p=Path("frontend/src/components/DriftGauge.tsx")
lines=p.read_text().splitlines()
for i in range(200, 260):
if i <= len(lines):
print(f"{i:4d}: {lines[i-1]}")
PY
echo
echo "== Test file outline/matches =="
rg -n "DriftGaugeGrid|rerender|assets=\{\[\]\}|assets=.*sampleAssets|empty" frontend/src/components/DriftGauge.test.tsx -C 2Repository: ritik4ever/stellar-portfolio-rebalancer
Length of output: 4392
🌐 Web query:
React Rules of Hooks keep hooks out of early or conditional return React throw
💡 Result:
React prohibits calling hooks after an early return because the library relies on a stable, consistent order of hook calls across every render [1][2]. When a component renders, React tracks the state of hooks by maintaining them in a fixed sequence—often visualized as a linked list [3]. If you place a hook after an early return (e.g., if a condition triggers a return before the hook is reached), that hook may be skipped in some renders but called in others [4][2]. This disrupts the order of the hook list, causing React to lose track of which state, effect, or context corresponds to which specific hook call [4][3]. If you break this rule, React will likely throw an error such as "Rendered fewer hooks than expected" [5]. This happens because React cannot distinguish between an intentional early return and an error where a hook call was accidentally placed inside a condition [5]. To fix this issue: - Always call hooks at the top level of your component or custom hook, before any conditional logic or early returns [1][6]. - If you need to perform an action conditionally, move the condition inside the hook itself (e.g., inside a useEffect) [4][2][3]. - If you need to return early from a component, ensure that all hooks are called unconditionally before that return statement [1][7]. The ESLint plugin 'eslint-plugin-react-hooks' is the standard tool for automatically detecting and preventing these violations in your codebase [1][8]. Note that the 'use' hook is an exception to these rules and can be called conditionally [4].
Citations:
- 1: https://react.dev/reference/rules/rules-of-hooks
- 2: https://legacy.reactjs.org/docs/hooks-rules.html
- 3: https://blog.atomrc.dev/p/why-you-cannot-condition-react-hooks/
- 4: https://uk.react.dev/reference/eslint-plugin-react-hooks/lints/rules-of-hooks
- 5: https://stackoverflow.com/questions/54938236/can-you-early-return-with-react-hooks
- 6: https://react.dev/reference/rules
- 7: https://stackoverflow.com/questions/79017939/conditionally-calling-a-hook-in-react
- 8: https://react.dev/warnings/invalid-hook-call-warning
Keep hooks before the empty-state return.
DriftGaugeGrid renders useState and then returns early for assets.length === 0; on a mounted rerender that returns populated assets, sortedAssets and totalDrift are added after an earlier render had fewer hooks, causing React to throw about hook order.
Proposed fix
const [showBreakdown, setShowBreakdown] = useState(false)
- if (assets.length === 0) return null
-
const sortedAssets = useMemo(() => {
return [...assets].sort((a, b) => Math.abs(b.current - b.target) - Math.abs(a.current - a.target))
}, [assets])
const totalDrift = useMemo(() => {
return assets.reduce((sum, a) => sum + Math.abs(a.current - a.target), 0)
}, [assets])
+
+ if (assets.length === 0) return null📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const [showBreakdown, setShowBreakdown] = useState(false) | |
| if (assets.length === 0) return null | |
| const sortedAssets = useMemo(() => { | |
| return [...assets].sort((a, b) => Math.abs(b.current - b.target) - Math.abs(a.current - a.target)) | |
| }, [assets]) | |
| const totalDrift = useMemo(() => { | |
| return assets.reduce((sum, a) => sum + Math.abs(a.current - a.target), 0) | |
| }, [assets]) | |
| const [showBreakdown, setShowBreakdown] = useState(false) | |
| const sortedAssets = useMemo(() => { | |
| return [...assets].sort((a, b) => Math.abs(b.current - b.target) - Math.abs(a.current - a.target)) | |
| }, [assets]) | |
| const totalDrift = useMemo(() => { | |
| return assets.reduce((sum, a) => sum + Math.abs(a.current - a.target), 0) | |
| }, [assets]) | |
| if (assets.length === 0) return null |
🤖 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/components/DriftGauge.tsx` around lines 230 - 240, Move the
assets-empty check in DriftGaugeGrid below the sortedAssets and totalDrift
useMemo hooks so every render invokes hooks in the same order. Preserve
returning null for empty assets while keeping the existing memoized calculations
unchanged.
| <h3 | ||
| id="drift-gauge-heading" | ||
| className="text-sm font-semibold text-gray-900 dark:text-white mb-4 cursor-default" | ||
| tabIndex={0} | ||
| > | ||
| {title} | ||
| </h3> | ||
|
|
||
| {showBreakdown && ( | ||
| <div | ||
| role="tooltip" | ||
| className="absolute bottom-full mb-1 left-0 z-50 w-64 rounded-lg bg-white dark:bg-gray-800 border border-gray-200 dark:border-gray-700 shadow-lg p-3 text-xs pointer-events-none" |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Find DriftGauge.tsx:"
fd -a 'DriftGauge\.tsx$' . || true
echo
echo "Outline:"
if [ -f frontend/src/components/DriftGauge.tsx ]; then
ast-grep outline frontend/src/components/DriftGauge.tsx --view expanded || true
echo
echo "Relevant lines 220-290:"
sed -n '220,290p' frontend/src/components/DriftGauge.tsx | cat -n -v
fi
echo
echo "Search Aria descriptions/usages nearby:"
rg -n "aria-describedby|tooltip|drift-gauge-heading|showBreakdown|useId|role=\"tooltip\"" frontend/src/components/DriftGauge.tsx frontend/src -S || trueRepository: ritik4ever/stellar-portfolio-rebalancer
Length of output: 8798
Associate the drift tooltip with its heading.
The keyboard-focusable drift-gauge-heading controls a role="tooltip" breakdown, but there is no aria-describedby linking them. Generate a stable tooltip id with useId() and reference it from the heading so assistive tech receives the drift details.
🤖 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/components/DriftGauge.tsx` around lines 254 - 265, Update the
DriftGauge component to generate a stable tooltip identifier with React useId,
assign it to the conditional breakdown element with role="tooltip", and add the
matching aria-describedby reference to the drift-gauge-heading element. Keep the
association valid when showBreakdown is false.
| const liveStepStatuses = useMemo(() => { | ||
| const statuses: Record<string, boolean> = {} | ||
| STEPS.forEach((step) => { | ||
| switch (step.id) { | ||
| case 'connect-wallet': | ||
| return !!publicKey | ||
| statuses[step.id] = !!publicKey | ||
| break | ||
| case 'create-portfolio': | ||
| return hasPortfolio | ||
| statuses[step.id] = hasPortfolio | ||
| break | ||
| case 'set-allocations': | ||
| return hasAllocations | ||
| statuses[step.id] = hasAllocations | ||
| break | ||
| case 'execute-rebalance': | ||
| return hasRebalanced | ||
| statuses[step.id] = hasRebalanced | ||
| break | ||
| case 'enable-auto-rebalance': | ||
| return hasAutoRebalance | ||
| statuses[step.id] = hasAutoRebalance | ||
| break | ||
| default: | ||
| return false | ||
| statuses[step.id] = false | ||
| } | ||
| }, | ||
| [publicKey, hasPortfolio, hasAllocations, hasRebalanced, hasAutoRebalance] | ||
| ) | ||
| }) | ||
| return statuses | ||
| }, [publicKey, hasPortfolio, hasAllocations, hasRebalanced, hasAutoRebalance]) | ||
|
|
||
| useEffect(() => { | ||
| if (!dismissed && !hasAutoShown && !allCompleted) { | ||
| const timer = setTimeout(() => setOpen(true), 800) | ||
| setHasAutoShown(true) | ||
| return () => clearTimeout(timer) | ||
| if (!initialLoadDone.current) { | ||
| initialLoadDone.current = true | ||
| const saved = loadCompletedSteps() | ||
| const merged = new Set(saved) | ||
| for (const [id, done] of Object.entries(liveStepStatuses)) { | ||
| if (done) merged.add(id) | ||
| } | ||
| setCompletedSteps(merged) | ||
| } | ||
| }, [liveStepStatuses]) | ||
|
|
||
| useEffect(() => { | ||
| if (!initialLoadDone.current) return | ||
| const updated = new Set(completedSteps) | ||
| let changed = false | ||
| for (const [id, done] of Object.entries(liveStepStatuses)) { | ||
| if (done && !updated.has(id)) { | ||
| updated.add(id) | ||
| changed = true | ||
| } | ||
| } | ||
| if (changed) { | ||
| setCompletedSteps(updated) | ||
| saveCompletedSteps(updated) | ||
| } | ||
| }, [dismissed, hasAutoShown, allCompleted]) | ||
| }, [liveStepStatuses]) | ||
|
|
||
| useEffect(() => { | ||
| saveCompletedSteps(completedSteps) | ||
| }, [completedSteps]) | ||
|
|
||
| const allCompleted = useMemo(() => STEPS.every((s) => completedSteps.has(s.id)), [completedSteps]) | ||
|
|
||
| const stepStatus = useCallback( | ||
| (id: string) => completedSteps.has(id), | ||
| [completedSteps] | ||
| ) | ||
|
|
||
| useEffect(() => { | ||
| if (dismissed || hasAutoShownRef.current || allCompleted) return | ||
| hasAutoShownRef.current = true | ||
| const timer = setTimeout(() => setOpen(true), 800) | ||
| return () => clearTimeout(timer) | ||
| }, [dismissed, allCompleted]) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Reset doesn't stick: unrelated live-status changes resurrect already-reset steps.
liveStepStatuses (Lines 100-124) is a brand-new object whenever any of its inputs (publicKey, hasPortfolio, hasAllocations, hasRebalanced, hasAutoRebalance) changes. The sync effect at Lines 138-152 re-runs on every such change and iterates all entries, re-adding any step that is still live-true and not yet in completedSteps — it doesn't distinguish "just became true" from "was already true before (and possibly reset)". So: user hits Reset (clears completedSteps), but if any other live condition changes afterward (e.g., a rebalance runs, allocations change) while, say, publicKey is still set, connect-wallet — which is still live-true — gets silently re-added, undoing part of the reset the user didn't ask to undo.
Separately, effect1 (Lines 126-136) and effect2 (Lines 138-152) both fire in the same initial commit (the ref guard flips synchronously), performing equivalent merges from the same source data — harmless but redundant duplicate work/persist on every mount.
Also note effect2 reads completedSteps from closure without listing it as a dependency (react-hooks/exhaustive-deps).
Suggest making the live→persisted sync edge-triggered (only add a step when it transitions from not-true to true), which also naturally merges the two effects into one:
♻️ Proposed consolidated, edge-triggered sync
- useEffect(() => {
- if (!initialLoadDone.current) {
- initialLoadDone.current = true
- const saved = loadCompletedSteps()
- const merged = new Set(saved)
- for (const [id, done] of Object.entries(liveStepStatuses)) {
- if (done) merged.add(id)
- }
- setCompletedSteps(merged)
- }
- }, [liveStepStatuses])
-
- useEffect(() => {
- if (!initialLoadDone.current) return
- const updated = new Set(completedSteps)
- let changed = false
- for (const [id, done] of Object.entries(liveStepStatuses)) {
- if (done && !updated.has(id)) {
- updated.add(id)
- changed = true
- }
- }
- if (changed) {
- setCompletedSteps(updated)
- saveCompletedSteps(updated)
- }
- }, [liveStepStatuses])
+ const prevLiveStatusesRef = useRef<Record<string, boolean> | null>(null)
+
+ useEffect(() => {
+ const prev = prevLiveStatusesRef.current
+ prevLiveStatusesRef.current = liveStepStatuses
+
+ if (!initialLoadDone.current) {
+ initialLoadDone.current = true
+ const merged = new Set(loadCompletedSteps())
+ for (const [id, done] of Object.entries(liveStepStatuses)) {
+ if (done) merged.add(id)
+ }
+ setCompletedSteps(merged)
+ return
+ }
+
+ setCompletedSteps((current) => {
+ let changed = false
+ const updated = new Set(current)
+ for (const [id, done] of Object.entries(liveStepStatuses)) {
+ const wasTrue = prev?.[id] === true
+ if (done && !wasTrue && !updated.has(id)) {
+ updated.add(id)
+ changed = true
+ }
+ }
+ return changed ? updated : current
+ })
+ }, [liveStepStatuses])Worth adding a regression test with a non-null publicKey (or another live-true condition) to cover the reset-then-unrelated-status-change scenario.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const liveStepStatuses = useMemo(() => { | |
| const statuses: Record<string, boolean> = {} | |
| STEPS.forEach((step) => { | |
| switch (step.id) { | |
| case 'connect-wallet': | |
| return !!publicKey | |
| statuses[step.id] = !!publicKey | |
| break | |
| case 'create-portfolio': | |
| return hasPortfolio | |
| statuses[step.id] = hasPortfolio | |
| break | |
| case 'set-allocations': | |
| return hasAllocations | |
| statuses[step.id] = hasAllocations | |
| break | |
| case 'execute-rebalance': | |
| return hasRebalanced | |
| statuses[step.id] = hasRebalanced | |
| break | |
| case 'enable-auto-rebalance': | |
| return hasAutoRebalance | |
| statuses[step.id] = hasAutoRebalance | |
| break | |
| default: | |
| return false | |
| statuses[step.id] = false | |
| } | |
| }, | |
| [publicKey, hasPortfolio, hasAllocations, hasRebalanced, hasAutoRebalance] | |
| ) | |
| }) | |
| return statuses | |
| }, [publicKey, hasPortfolio, hasAllocations, hasRebalanced, hasAutoRebalance]) | |
| useEffect(() => { | |
| if (!dismissed && !hasAutoShown && !allCompleted) { | |
| const timer = setTimeout(() => setOpen(true), 800) | |
| setHasAutoShown(true) | |
| return () => clearTimeout(timer) | |
| if (!initialLoadDone.current) { | |
| initialLoadDone.current = true | |
| const saved = loadCompletedSteps() | |
| const merged = new Set(saved) | |
| for (const [id, done] of Object.entries(liveStepStatuses)) { | |
| if (done) merged.add(id) | |
| } | |
| setCompletedSteps(merged) | |
| } | |
| }, [liveStepStatuses]) | |
| useEffect(() => { | |
| if (!initialLoadDone.current) return | |
| const updated = new Set(completedSteps) | |
| let changed = false | |
| for (const [id, done] of Object.entries(liveStepStatuses)) { | |
| if (done && !updated.has(id)) { | |
| updated.add(id) | |
| changed = true | |
| } | |
| } | |
| if (changed) { | |
| setCompletedSteps(updated) | |
| saveCompletedSteps(updated) | |
| } | |
| }, [dismissed, hasAutoShown, allCompleted]) | |
| }, [liveStepStatuses]) | |
| useEffect(() => { | |
| saveCompletedSteps(completedSteps) | |
| }, [completedSteps]) | |
| const allCompleted = useMemo(() => STEPS.every((s) => completedSteps.has(s.id)), [completedSteps]) | |
| const stepStatus = useCallback( | |
| (id: string) => completedSteps.has(id), | |
| [completedSteps] | |
| ) | |
| useEffect(() => { | |
| if (dismissed || hasAutoShownRef.current || allCompleted) return | |
| hasAutoShownRef.current = true | |
| const timer = setTimeout(() => setOpen(true), 800) | |
| return () => clearTimeout(timer) | |
| }, [dismissed, allCompleted]) | |
| const liveStepStatuses = useMemo(() => { | |
| const statuses: Record<string, boolean> = {} | |
| STEPS.forEach((step) => { | |
| switch (step.id) { | |
| case 'connect-wallet': | |
| statuses[step.id] = !!publicKey | |
| break | |
| case 'create-portfolio': | |
| statuses[step.id] = hasPortfolio | |
| break | |
| case 'set-allocations': | |
| statuses[step.id] = hasAllocations | |
| break | |
| case 'execute-rebalance': | |
| statuses[step.id] = hasRebalanced | |
| break | |
| case 'enable-auto-rebalance': | |
| statuses[step.id] = hasAutoRebalance | |
| break | |
| default: | |
| statuses[step.id] = false | |
| } | |
| }) | |
| return statuses | |
| }, [publicKey, hasPortfolio, hasAllocations, hasRebalanced, hasAutoRebalance]) | |
| const prevLiveStatusesRef = useRef<Record<string, boolean> | null>(null) | |
| useEffect(() => { | |
| const prev = prevLiveStatusesRef.current | |
| prevLiveStatusesRef.current = liveStepStatuses | |
| if (!initialLoadDone.current) { | |
| initialLoadDone.current = true | |
| const merged = new Set(loadCompletedSteps()) | |
| for (const [id, done] of Object.entries(liveStepStatuses)) { | |
| if (done) merged.add(id) | |
| } | |
| setCompletedSteps(merged) | |
| return | |
| } | |
| setCompletedSteps((current) => { | |
| let changed = false | |
| const updated = new Set(current) | |
| for (const [id, done] of Object.entries(liveStepStatuses)) { | |
| const wasTrue = prev?.[id] === true | |
| if (done && !wasTrue && !updated.has(id)) { | |
| updated.add(id) | |
| changed = true | |
| } | |
| } | |
| return changed ? updated : current | |
| }) | |
| }, [liveStepStatuses]) | |
| useEffect(() => { | |
| saveCompletedSteps(completedSteps) | |
| }, [completedSteps]) | |
| const allCompleted = useMemo(() => STEPS.every((s) => completedSteps.has(s.id)), [completedSteps]) | |
| const stepStatus = useCallback( | |
| (id: string) => completedSteps.has(id), | |
| [completedSteps] | |
| ) | |
| useEffect(() => { | |
| if (dismissed || hasAutoShownRef.current || allCompleted) return | |
| hasAutoShownRef.current = true | |
| const timer = setTimeout(() => setOpen(true), 800) | |
| return () => clearTimeout(timer) | |
| }, [dismissed, allCompleted]) |
🤖 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/components/OnboardingChecklist.tsx` around lines 100 - 170,
Update the liveStepStatuses synchronization in OnboardingChecklist so persisted
steps are added only when a live status transitions from false to true,
preventing unrelated status changes from resurrecting steps cleared by Reset.
Consolidate the initial-load merge and ongoing sync into one effect, track
previous live statuses, and include all referenced state in dependencies without
relying on a stale completedSteps closure. Add a regression test covering reset
followed by an unrelated live-status change while another condition remains
true.
| const POLL_MS = 45_000 | ||
| const HIDDEN_POLL_MS = POLL_MS * 4 | ||
|
|
||
| /** | ||
| * React Query version of useReadinessReport. | ||
| * Replaces manual setInterval polling with TanStack Query refetchInterval. | ||
| * Polling frequency adapts based on tab visibility (backoff when hidden). | ||
| */ | ||
| export function useReadinessQuery() { | ||
| const { data: report, isLoading: loading, isError: loadError, refetch: refresh } = useQuery({ | ||
| queryKey: readinessKeys.all, | ||
| queryFn: fetchReadinessReport, | ||
| refetchInterval: POLL_MS, | ||
| refetchInterval: () => document.visibilityState === 'visible' ? POLL_MS : HIDDEN_POLL_MS, |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C 3 'refetchInterval(InBackground)?|visibilitychange|useReadinessQuery' .Repository: ritik4ever/stellar-portfolio-rebalancer
Length of output: 10524
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "## useReadinessQuery.ts"
cat -n frontend/src/hooks/queries/useReadinessQuery.ts | sed -n '1,120p'
echo
echo "## TanStack Query dependency versions"
for f in package.json frontend/package.json pnpm-lock.yaml package-lock.json yarn.lock; do
if [ -f "$f" ]; then
echo "--- $f ---"
rg -n '`@tanstack`|useQuery|query-key' "$f" | head -80 || true
fi
done
echo
echo "## Nearby old visibility polling implementation"
cat -n frontend/src/hooks/useReadinessReport.ts | sed -n '160,210p'Repository: ritik4ever/stellar-portfolio-rebalancer
Length of output: 6022
🌐 Web query:
TanStack Query v5 refetchIntervalInBackground default pause interval fetches in background tabs
💡 Result:
In TanStack Query v5, polling is paused by default when the browser tab loses focus [1][2]. The refetchIntervalInBackground option is used to override this behavior; when set to true, queries configured with a refetchInterval will continue to refetch even while the browser tab is inactive in the background [3][4]. However, it is important to note that this setting specifically governs interval-based refetching. Other automatic background processes, such as query retries, may still pause when the tab is inactive because they are tied to window focus events independently of the refetchIntervalInBackground configuration [5][6]. Summary of behavior: - Default: Polling pauses when the tab loses focus [1][2]. - With refetchIntervalInBackground: true: Polling continues while the tab is in the background [3][4]. - Note on Retries: Retries currently remain subject to tab focus behavior, meaning they may pause in the background even if refetchIntervalInBackground is enabled for your periodic polling [5][6].
Citations:
- 1: https://tanstack.com/query/v5/docs/framework/react/guides/polling
- 2: https://tanstack.com/query/latest/docs/framework/react/guides/polling
- 3: https://tanstack.com/query/latest/docs/framework/react/reference/useQuery
- 4: https://tanstack.com/query/v5/docs/framework/react/reference/useQuery
- 5: https://tanstack.com/query/v5/docs/framework/react/guides/query-retries
- 6: Polling Stops with refetchIntervalInBackground and Retry When Tab Is Inactive TanStack/query#8353
Make visibility changes actually affect the hidden polling interval.
With TanStack Query’s default behavior, interval refetches are paused when the tab loses focus, so the HIDDEN_POLL_MS slowdown is not reached; if this hidden-rate behavior is needed, pass refetchIntervalInBackground: true. Also, reading document.visibilityState directly in refetchInterval does not subscribe the query to visibilitychange, so the current interval may not switch on visibility changes. Use a React-visible isVisible state updated from a SSR-safe visibilitychange listener and pass that into refetchInterval.
🤖 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/hooks/queries/useReadinessQuery.ts` around lines 57 - 69, The
useReadinessQuery polling interval does not reliably adapt to tab visibility
changes. Add SSR-safe React visibility state updated by a visibilitychange
listener, use that isVisible state in refetchInterval to select POLL_MS or
HIDDEN_POLL_MS, and enable refetchIntervalInBackground so hidden-tab polling
continues at the slower rate.
Source: MCP tools
2 participants
closes #1457
closes #1458
closes #1459
closes #1460
Summary by CodeRabbit