# PR #161 Code Review — `fix: WS reconnect when foregrounded without active subscribers` **Reviewer:** tele **Date:** 2026-04-19 **PR:** https://github.com/NanoStreetApp/mobile/pull/161 **HEAD:** `f7622a4f` • Branch `fix/ws-foreground-restore` **Diff:** 2 files, +133/-4 (production: +5/-4 in `MarketWebSocketProvider.tsx`; tests: +128 new file) --- ## TL;DR — APPROVE with one minor follow-up to consider The fix matches Option A from the bug report exactly, is minimal (5 LOC of real change), well-commented, and the test file is clean and well-factored. PR description is human-readable and follows the project's conventions. **Safe to mark ready and merge.** One small post-fix side effect is worth being aware of but doesn't block. --- ## Implementation review (`MarketWebSocketProvider.tsx`) **Diff (5 LOC real change):** - Removed `shouldConnectRef` parameter from `useWsAppStateSync` signature - Removed the `shouldConnectRef.current` check in the foreground guard, leaving `if (nextState === 'active' && !connectedRef.current)` - Updated callsite in `useWsConnectionLifecycle` - Added a clear inline comment explaining the symmetry rationale ✅ **Match against bug report:** Exact Option A. Cold-start mount (`useWsConnectionLifecycle` line 50-52) is unconditional; foreground reconnect is now also unconditional. Symmetry restored. ✅ **Comment quality:** The added comment ("Foreground reconnect is symmetric with cold-start mount — unconditional. useWsMarketStatusTeardown handles the 'market closed' teardown post-reconnect.") is exactly the kind of WHY comment the project's coding-standards encourage. Future readers won't have to reverse-engineer the design intent. ✅ **No collateral damage:** subscribe/unsubscribe paths untouched, `useWsMarketStatusTeardown` left intact (10s grace teardown still fires for closed markets), `WebSocketClient` reconnect/backoff untouched, `onStateChange → resubscribeAll` resubscribe path preserved. ✅ **Layer rules respected:** Change is fully inside `presentation/providers/`. No business/data/service/infra leakage. ✅ **File size:** ~181 lines after the change, well under the 350-line max. ## Test review (`MarketWebSocketProvider.test.tsx`) 128 new lines, 2 tests, both well-named and minimal-mock. ### Test 1 — `reconnects on foreground even when market is closed (bug fix)` Sets `marketStatus = 'closed'`, renders provider, asserts: - Initial mount calls `connect()` once (cold-start path is unconditional, sanity check) - After firing `connected` state → background AppState event → asserts `disconnect()` called - After firing `disconnected` state → foreground AppState event → asserts `connect()` called a SECOND time ✅ This is THE bug-fix test. Pre-fix this would fail (second `connect()` would never fire because `shouldConnectRef.current === false`). ### Test 2 — `resubscribes tracked tickers after foreground reconnect` Renders with `['AAPL', 'MSFT']`, walks through the full cycle, asserts subscribe is called with the original tickers BOTH on initial connect AND after the foreground reconnect. ✅ Regression guard. Confirms the `onStateChange → resubscribeAll` path is still wired correctly post-fix. The two tests together prove "foreground recovery is end-to-end" (reconnect AND resubscribe). ### Test infrastructure ✅ Mocks at correct boundaries — `container.livePrice` (DI-level), `useMarketStatusQuery` (hook-level), `AppState.addEventListener` (RN API). No internal-module mocking. ✅ Clean helpers (`renderProvider`, `fireAppState`, `fireConnectionState`, `Subscriber`) — readable and reusable. ✅ Proper teardown: `jest.useFakeTimers()` + `jest.useRealTimers()` in beforeEach/afterEach, `jest.restoreAllMocks()`, listener refs reset. ### One tiny test-file nit (NOT a blocker) Per `CLAUDE.md` Coding Standards: "In test files that use `jest.mock`, place non-type imports before type imports and do not include an empty line within the import group." Current order: ```tsx import { act, render } from '@testing-library/react-native'; // non-type import React, { useEffect } from 'react'; // non-type import type { AppStateStatus } from 'react-native'; // type ← interleaved import { AppState, Text } from 'react-native'; // non-type ``` The `import type` for `AppStateStatus` is interleaved with the `react-native` non-type import on the next line. Per the rule, all non-type should come first. Fix is one line move — group both `react-native` lines: ```tsx import { AppState, Text } from 'react-native'; import type { AppStateStatus } from 'react-native'; ``` `rtk pnpm lint --fix` may auto-resolve this. Trivial. ## Edge case worth flagging — NOT a blocker, but discussable Post-fix introduces a small new behavior on a specific timeline: **if the user is backgrounded across the open→closed market transition, foregrounding will reconnect AND stay connected during off-hours indefinitely.** Walkthrough: - t=0: market open, foregrounded, connected. `marketStatus='open'`, no teardown timer. - t=10: user backgrounds. AppState handler disconnects. `connectedRef=false`. - t=15: market closes. `marketStatus` flips to `'closed'`. `useWsMarketStatusTeardown` re-runs, sets `shouldConnectRef.current=false`, but the timer-arming branch requires `connectedRef.current=true` — which is false right now. **No timer armed.** - t=20: user foregrounds. AppState handler now reconnects unconditionally (the fix). `connect()` fires, state goes `connected`, `connectedRef.current=true`. - t=20+: `useWsMarketStatusTeardown` does NOT re-run because none of its deps changed (refs are not React state). **Connection persists during closed market until the next `marketStatus` value change.** Pre-fix this didn't manifest because the foreground handler refused to reconnect during closed markets in the first place. **Why it's NOT urgent:** - Off-hours WS connections are cheap; the user actively foregrounded, so they presumably want live data - Bug only triggers under a specific timing (background→close-transition→foreground); typical user behavior is foreground while market is already closed (existing case still teardown-armed correctly via the t=0 mount path) - Synthetic backend exposure is the same as before — useful for validating **Suggested follow-up (NOT in this PR):** in `useWsConnectionLifecycle`'s `onStateChange` handler (line 56-64), when transitioning from `disconnected → connected` AND `shouldConnectRef.current === false`, re-arm the 10s teardown. Or simpler: add a `connectedRef`-flip-aware effect that re-evaluates teardown on every transition. ~10 LOC, separate small PR. This is worth tracking but doesn't gate this PR — the original bug user reported (no reconnect at all) is fully fixed, and the new edge case is strictly a longer-than-necessary connection during off-hours, not a broken user experience. ## What this PR does NOT change (verified) - Subscribe/unsubscribe API surface — unchanged - `useWsMarketStatusTeardown`'s 10s grace teardown — preserved - `WebSocketClient` reconnect/backoff — preserved - `onStateChange → resubscribeAll` resubscribe path — preserved (regression-tested) - No "Markets closed — prices paused" UI indicator (out of scope, flagged in bug report) - No synthetic backend market-status override (out of scope, flagged separately) ## Summary | Aspect | Verdict | |---|---| | Implementation correctness | ✅ Matches Option A exactly | | Code quality | ✅ Minimal, well-commented | | Test coverage | ✅ Bug repro + regression guard, both pass deterministically | | Layer / file-size rules | ✅ All respected | | PR description | ✅ Human-readable, complete | | Risk | ✅ Low — narrow change, deterministic tests | | Open question | One minor follow-up (closed-market reconnect persistence) — not a blocker | | Test-file lint nit | One import-order fix, trivial | **Recommendation: APPROVE.** Mark ready, merge when convenient. Optionally apply the import-order auto-fix before merge. Open a small follow-up issue/PR for the post-reconnect teardown re-arm if you agree it's worth fixing.