# PR Review: #211, #213, #214, #215 **Date:** 2026-04-28 **Reviewer:** coor (read-only review) **Scope:** all four PRs are mobile, against `main` (post `2d3cd081`). --- ## TL;DR | PR | Title (short) | Risk | Recommendation | |-----|----------------------------------------------------------------|---------|--------------------------------------------------------------------------------| | 211 | Background 1D refresh + foreground sync | Medium | Land after concerns 1 + 4 below are addressed (test + scope of invalidation). | | 213 | Hermes + worklets-core off-thread + push ticker | High | Land after worklet-correctness review + on-device perf + flicker check. | | 214 | Stale 1D chart fix (market-aware refetch + unmount invalidate) | Low | Land. Smallest, cleanest of the four. | | 215 | Enable React Compiler, strip manual memoization | High | Hold until verified on device profiles; carries silent perf regression risk. | --- ## PR #211 — `feat/background-refresh` **Title:** feat(bg-refresh): background 1D chart refresh + foreground cache invalidation **Size:** +118 / −1 (6 files) **Files:** `app.json`, `app/_layout.tsx`, `package.json`, `pnpm-lock.yaml`, `src/core/startup/backgroundTasks.ts` (new), `src/core/startup/useAppForegroundSync.ts` (new) ### What it does - Adds `expo-background-fetch` + `expo-task-manager` to plugins - `backgroundTasks.ts` defines a 15-min minimum-interval task at module top level (per Expo TaskManager API — must register before render). Reads watchlist tickers from `home_watchlist_tickers` AsyncStorage key, fetches 1D aggs via `container.market.repository.getStockAggs`, writes results into `appQueryClient` cache. - `useAppForegroundSync` watches `AppState` and invalidates all `['market', 'stock-aggs']` queries on background→active transition (refetchType: 'active' — refetches mounted queries only). - Wires both into `app/_layout.tsx`. ### Architecture - ✅ Layer rules respected. `core/startup/` reaching into `container.market.repository` is fine — container is the DI surface, the file already does this for other startup concerns (see `prefetchHomeData.ts`). - ✅ Magic AsyncStorage key `home_watchlist_tickers` is consistent with `prefetchHomeData.ts:13` (already the same constant). Cross-feature coupling is explicit and pre-existing. ### Concerns 1. **No tests for the new code.** `prefetchHomeData.test.ts` exists for the sister file — same style of test (mocked AsyncStorage + container) is achievable here. At minimum: task-fn happy path, no-tickers branch (`NoData`), partial failure branch. Skipping tests on a code path that runs invisibly in the OS scheduler is risky. 2. **iOS background-fetch is a hint, not a contract.** The 15-min minimum is the floor; iOS schedules at its discretion based on user habits, battery, and last-foregrounded time. A few users will rarely if ever see the task fire. PR description should document this expectation; the comment block in `backgroundTasks.ts` should call it out. 3. **Direct `setQueryData` couples the task to query-key shape.** `['market', 'stock-aggs', ticker, '1D']` is also written from `useHistoricalDataViewModel.ts`, `useHistoricalChartViewModel.ts`, `useChartData.ts`, `useResilientSnapshot.test.ts`. If the key shape ever changes, this background-write goes silent. Prefer a small helper (e.g. `chartAggsQueryKey(ticker, timeRange)`) shared with the consumers — even better, a typed `prefetchQuery` call so React Query owns the key. 4. **Foreground invalidate scope is broad.** `invalidateQueries({ queryKey: ['market', 'stock-aggs'] })` matches every range/timeframe (1D, 1W, 1M, 1Y, etc.) for every ticker. After a long backgrounded app return, this can blow up into many concurrent refetches across all open stock-detail screens + sparklines + watchlist + market-mover. Either: - Narrow to active 1D queries only via a `predicate: q => q.queryKey[3] === '1D'`, or - Rely on PR #214's `refetchInterval` instead and drop this hook. 5. **`subscription.remove()` cleanup is correct.** `AppState.addEventListener` returns a `Subscription` in modern RN; `.remove()` is the documented teardown. ### Suggested before merge - Add tests for the task fn (mirror `prefetchHomeData.test.ts`). - Tighten the foreground invalidation scope. - Either share the query-key helper with consumers OR add a type-level test asserting the tuple shape. --- ## PR #213 — `feat/ui-thread-crosshair` **Title:** perf(threads): UI-thread crosshair, push ticker subscribe, Hermes, news dedup O(n) **Size:** +188 / −92 (15 files) **Files:** `app.json`, `babel.config.js`, `jest.setup.cjs`, `package.json`, `pnpm-lock.yaml`, plus business utils (`livePriceUtils.ts`, `sessionBands.ts`, `stitchSessionBars.ts`, `etClock.ts`), news repository, and chart hooks/components. ### What it does - **Hermes**: enabled in both `ios` and `android` blocks of `app.json`. - **Push ticks**: `useChartData.useThrottledLiveTick` replaces 1Hz `setInterval` poll with `livePriceStore.subscribeTicker` push events. A `useRef` timestamp gates `setSampled` to ≤1Hz. - **UI-thread crosshair**: in `InteractiveLineChart.tsx`, removes `useState` / `useAnimatedReaction` / `runOnJS`. Price label now uses `useDerivedValue` + `useAnimatedProps` on an `AnimatedTextInput` with `editable={false}` — text updates without crossing the JS bridge. - **Worklets-core**: adds `react-native-worklets-core@1.6.3`, plugin in babel config (BEFORE reanimated plugin). Heavy synchronous work (`stitchSessionBars`, `filterSortAndAggregate`) moved to `Worklets.runAsync` contexts (`_stitchCtx`, `_filterCtx`, `_aggregateCtx`). - **`'worklet'` directives** added to many functions in `livePriceUtils`, `sessionBands`, `stitchSessionBars`, `etClock`, and inline aggregator helpers. - **News dedup O(n)**: `dedupeById(items.map(mapper))` → single-pass `mapAndDedupeById(items, mapper)` that maps and dedups in one loop. ### Architecture - ✅ `'worklet'` is a string literal directive; it does not introduce a framework import. `business/utils/` files remain framework-agnostic. - ✅ `react-native-worklets-core` imports live in presentation (hooks + components). Layer rules intact. - ✅ Babel plugin order (`react-native-worklets-core/plugin` before `react-native-reanimated/plugin`) matches the documented requirement. ### Concerns 1. **`useMemo` → `useEffect + setState` via Worklets contexts changes paint timing.** Examples: `StockLineChart.useChartSeries` (stitched), `useHistoricalDataViewModel` (aggregated bars). On the first render of any ticker / range change, the worklet result is `null` / `[]` until the async-context resolves. Result: a brief empty/flicker frame where there used to be content. For 1D session-tinted charts this is the most visible path. Mitigation: render a pre-stitch fallback (e.g. plain colored line) while the worklet result is pending. Current code falls back to `null` (StockLineChart) or `[]` (HistoricalDataViewModel). 2. **Worklet-safety of utility functions is asserted by directive, not verified.** Every function tagged `'worklet'` must (a) only call other worklet-safe functions, (b) only reference shareable values, (c) avoid object identity tricks. The Jest mock (`jest.setup.cjs`) makes `runAsync` synchronous via `Promise.resolve(fn())`, so any non-worklet-safe code path will pass tests but crash on device. Recommended: at least one device smoke test in the PR notes (run app, exercise stock detail line + candle chart, range change, watchlist nav). 3. **`AnimatedTextInput` for crosshair text** is the documented Reanimated workaround for UI-thread text without Skia, but it is brittle: any future styling change (font size override, lineHeight inheritance, accessibility) on `crosshairText` style needs to keep `padding: 0`, `borderWidth: 0`, `backgroundColor: 'transparent'` in lockstep. Add a comment block above the style entry explaining why those three lines exist or future-coor will "clean them up." 4. **News dedup behavior change is subtle but correct.** Old: map all → dedup. New: map+dedup in one loop. Equivalent output, fewer allocations. Single-pass ordering preserved (Set + array). Good change. 5. **Hermes change is a runtime swap.** With SDK 54, Hermes should be the default already on most native shapes; if Expo previously had `jsEngine: jsc` somewhere implicit, this is a behavior change that affects: (a) date-fns/Intl behavior, (b) stack traces in Sentry-like tools, (c) Number precision near edges. Worth a smoke pass on chart math (decimals near $0.001 ticks, % formatting). 6. **No test added for `useThrottledLiveTick` in subscribe-mode.** Existing test was updated for the new shape but the throttle gate (`now - lastUpdateRef.current < LIVE_MERGE_INTERVAL_MS`) has no direct coverage. A 2-tick test (fire two pushes 100ms apart, assert second is dropped) would close the gap. 7. **Type narrowing via `!` in `useChartData.ts`.** `usingPrimaryBars ? primaryBars! : fallback.bars` — `primaryBars` is now `readonly AggBar[] | null`. The `!` narrows correctly because `usingPrimaryBars = (primaryBars?.length ?? 0) > 0` implies non-null, but a destructured guard or `?? fallback.bars` would read cleaner without the assertion. ### Suggested before merge - Add a no-flicker fallback for stitched/aggregate computation while worklet result is pending (or document the flicker as acceptable and link to a follow-up ticket). - One paragraph in the PR body: "tested on iOS device + Android emulator: range switching, session-tinted line, candle, historical screen — no crashes." - Add throttle test for `useThrottledLiveTick`. - Comment the crosshair `AnimatedTextInput` style rationale. --- ## PR #214 — `fix/stale-chart-data` **Title:** fix(chart): stale 1D chart data — market-aware refetch + unmount invalidation **Size:** +29 / −13 (3 files) **Files:** `useChartData.test.ts`, `useChartData.ts`, `StockDetailScreen.tsx` ### What it does - Splits `CHART_STALE_TIME` into open (60s) and closed (5min) variants for the 1D query. - Adds `refetchInterval: 60000` while market is active. - `isDataStale` flips logic: instead of "1D refetch in flight + filter empty", now "market is active + last bar pre-dates today (ET)". Surfaces a spinner over old bars. - Adds an unmount-time `queryClient.invalidateQueries({ refetchType: 'none' })` for the 1D key on the stock-detail screen — marks stale without firing a refetch, so re-mount picks up fresh data. ### Architecture - ✅ `useMarketStatusQuery` is exported from `@/features/market` (verified at `src/features/market/index.ts:46`). Cross-feature use via public barrel is correct. - ✅ `getEtDateKey` from `@/shared/time/etClock` — shared util, OK. ### Concerns 1. **`useMarketStatusQuery()` returning `null`** is treated as `isMarketActive = false` (since `null !== 'open'/'pre-market'/'after-hours'`). That means until the first market-status fetch resolves, the 1D query uses the *closed* staleTime (5min). On a cold launch during open hours the chart may show a slightly stale bar for one cycle. Probably acceptable; worth a comment near `isMarketActive`. 2. **Polling cost.** A 60s refetchInterval per opened stock-detail across the app is fine in absolute terms but compounds with PR #211's foreground invalidation (which then forces a refetch on every mounted 1D query). If both land, expect a refetch storm on each foreground. Coordinate with #211 (see #211 concern 4). 3. **Test coverage.** A new mock entry is added but no new test asserts the open-hours refetchInterval is wired. Light gap; not blocking. ### Suggested before merge - Add a comment near `isMarketActive` documenting the `null` → `closed` fallback behavior. - Decide ordering vs #211 (this can land first cleanly, then #211 narrows scope). --- ## PR #215 — `feat/react-compiler` **Title:** feat(compiler): enable React Compiler — auto-memoization replaces manual useMemo/useCallback/memo **Size:** +79 / −125 (14 files) **Files:** `app.json`, `package.json`, `pnpm-lock.yaml`, plus ten files under `src/features/{market,news,stock-detail}/presentation/` ### What it does - Sets `experiments.reactCompiler: true` in `app.json`. - Adds `babel-plugin-react-compiler@^1.0.0` as devDependency (relies on `babel-preset-expo` to wire it). - Strips manual memoization across hot paths: - `ChartSlot`, `StockLineChart`, `StockCandleChart` — drops outer `memo` wrapper. - `LineChartCanvas`, `CandleChartCanvas` — drops inner `memo` wrappers. - `TabContent` (in `StockDetailScreen.tsx`) — drops `memo`. - `useHapticIndexChange` returns plain function instead of `useCallback`. - News hooks (`useInfiniteNewsQuery`) drop `useMemo` for articles + sortedTickersKey. - `useOverviewViewModel`, `useQuoteMetricsViewModel`, `useHistoricalDataViewModel`, `useHistoricalChartViewModel` drop `useMemo` and `useCallback` throughout. - `MarketOverviewSection` drops `useMemo` for `indexSymbols`. ### Architecture - ✅ No layer changes. Removing memoization preserves dependency directions. ### Concerns 1. **High blast radius for a v1.0.0 plugin.** `babel-plugin-react-compiler@1.0.0` is a recent first-stable. The healthcheck output ("1784/1784 components compile cleanly") is encouraging but only covers static analysis — runtime correctness on device is the actual gate. There is no dev-vs-prod parity guarantee (compiler may behave differently across bundler configs). 2. **Hot-path components had the `memo` wrappers for measured reasons.** `ChartSlot`, `StockLineChart`, `StockCandleChart` re-render on every parent state change in stock-detail, which is heavy (collapsible header animations, snapshot ticks, scroll). If the compiler does not memoize these *exactly* the same shape, the lost wrapper is a perf regression invisible in release builds until it bites a user with a low-end Android. 3. **Mixed-paradigm risk during rollout.** Once #213 lands (UI-thread crosshair, worklets, Hermes) and #215 lands (compiler), the chart layer has three new things at once: thread split, JS engine, auto-memo. Bisecting any future perf regression becomes harder. Prefer landing #215 *after* #213 has soaked, OR landing #215 first behind a build-time flag that can be flipped off if regression appears. 4. **No new tests, no perf comparison.** PR description says "compiles cleanly" — necessary but not sufficient. At minimum: a profiling capture (Flipper / React DevTools) of stock-detail scroll + range switch before / after, attached to the PR body. Without it, "remove memoization, trust compiler" is a faith move. 5. **`useHapticIndexChange` returning a plain function** means a new function reference each render. Compiler should memoize where used (`onIndexChange` prop on chart canvases). Verify the canvases don't re-render on each prop identity change (they will if compiler doesn't catch it). 6. **Removed `useCallback` for `applyPreset`, `setActiveLevel`, `retry`** — these are passed to children as props. Same compiler-trust caveat. Worth checking that `OverviewContent`, `HistoricalDataScreen` don't visibly re-render extra on every parent update. 7. **`articles: deduplicateArticles(...)` runs every render.** With #213's news dedup change folded in (single pass, but still O(n)), unmemoized re-execution on every render of a news-screen container is acceptable for short lists, expensive for long infinite-scroll lists. Compiler should catch and memoize on `query.data` identity — verify in profiler. ### Suggested before merge - Hold until #213 has soaked on main (see concern 3). Order: #214 → #213 → #211 → #215. - Attach before/after profiling for at least: stock-detail scroll + range switch, news infinite-scroll, watchlist load. - Smoke on a low-end Android device specifically. Compiler bugs tend to surface where memoization mattered most. --- ## Cross-PR observations 1. **All four touch chart-data / market-data hot path.** Conflicts are mechanical, not semantic, but landing all four in the same week is a recipe for a "which one regressed?" investigation. Suggested merge order: - **#214 first** (smallest, narrow fix, low risk, immediate user-visible benefit). - **#213 second** (Hermes + worklets is foundational; lots of code paths get faster but also riskier). - **#211 third** (relies on #214's market-aware refetch behavior to coordinate scope). - **#215 last** (compiler benefits compound on top of stable hot path; easier to attribute regressions). 2. **Test coverage trends downward across the set.** #211 adds zero new tests; #215 strips memoization without behavior tests. #213 updates one test for the subscribe shape but doesn't cover the throttle. #214 is the only PR with proportionate test work. 3. **No PR includes EAS preview QR or device-test note in the body.** Per project rule (`feedback_every_pr_needs_eas_preview.md`), every mobile PR needs an EAS preview comment before review — the PostToolUse hook should have posted it. Verify each PR has a preview comment; if not, post manually. 4. **All four assume `main` post-#208 and post-merged eas-hook-fix (#207).** The hook fix is in main, so future pushes to these branches should produce preview comments automatically. --- ## File-by-file flag list ### Architecture violations - None found across all four PRs. Layer rules respected. ### Type safety - #213 `useChartData.ts`: `usingPrimaryBars ? primaryBars! : fallback.bars` uses non-null assertion. The `!` narrows correctly because `usingPrimaryBars` proves non-null, but a destructured guard or `?? fallback.bars` would read cleaner. ### Tests - #211: zero added; recommend mirroring `prefetchHomeData.test.ts`. - #213: existing test updated, no new test for subscribe-throttle. - #214: new mock entry, no new behavioral test for refetchInterval. - #215: zero added; recommend profiling captures. ### Style / coding standards - All four respect import order, file-size, and function-size limits as far as the diff shows. - No new default exports introduced (good). - No `console.log` introduced. --- ## Recommended merge sequence 1. **Today**: #214 (after the comment-fix above) 2. **+1 day soak**: #213 (after worklet fallback for first-paint flicker is added or accepted, plus device smoke note) 3. **+1 day soak**: #211 (after foreground-invalidation scope is narrowed and tests added) 4. **+2 day soak**: #215 (after profiling captures attached and #213 has been on main long enough to attribute any compiler regression unambiguously)