fix(weather-alerts): resolve iOS screen freeze on navigation#2499
Merged
Conversation
Three issues caused the screen to freeze on iOS when tapping Weather Alerts: 1. `presentation: 'card'` + `animation: 'slide_from_bottom'` mismatch in _layout.tsx. Every other slide-from-bottom screen uses `presentation: 'modal'`. The conflicting config caused the native iOS navigation controller to set up a left-edge pan gesture (card) while the screen arrived from the bottom, leaving the screen unresponsive. Fixed by aligning to `presentation: 'modal'`. 2. `contentInsetAdjustmentBehavior="automatic"` on the ScrollView with `headerShown: false` caused iOS to compute incorrect content insets (no native navigation bar exists to measure). Changed to `"never"`. 3. `useWeatherAlerts` depended on the whole `activeLocation` object, which is recreated on every Jotai atom evaluation via `.find()`. Extracted primitive `locationId`/`locationName` as dependencies to prevent spurious re-fetches. Added a `cancelled` cleanup flag to discard stale async results on unmount. Added Sentry breadcrumb and error capture per instrumentation convention. Closes #2498
Contributor
WalkthroughThis PR fixes a screen freeze when navigating to Weather Alerts by simplifying the WeatherAlertsTile component to remove problematic UI fallback rendering, adding async cleanup in useWeatherAlert to prevent post-unmount state updates, adjusting the screen's animation behavior and layout spacing, and integrating Sentry instrumentation for error monitoring. ChangesWeather Alerts Navigation and Stability
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint skipped: no ESLint configuration detected in root package.json. To enable, add 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 |
With presentation:'modal' the dashboard screen stays fully active behind the modal, so WeatherAlertsTile keeps rendering. The Alert component from NativeWindUI received new object literals for materialIcon and buttons on every render and used them inside its own internal effects, which drove the "Maximum update depth exceeded" loop. The Alert and its ref were dead code — alertRef.current was never called anywhere in the component. Removed both entirely.
…kground-screen loop presentation:'modal' keeps the dashboard screen fully active behind the overlay, which caused NativeWindUI components (ListItem and others on the dashboard) to drive a re-render loop via their internal effects on unstable prop references — surfacing as "Maximum update depth exceeded". The original screen freeze was caused by the hook's unstable [activeLocation] object dependency (already fixed in a prior commit), not by the presentation type. Reverting to presentation:'card' deactivates background-screen rendering on iOS and eliminates the loop. Also corrected the animation from 'slide_from_bottom' (modal idiom) to 'default' (standard push-right for card) for consistency.
LargeTitleHeader from NativeWindUI creates a native iOS navigation bar. Setting the ScrollView to "never" prevented iOS from accounting for that bar, causing the content to overlap the header. Restored to "automatic" so iOS adjusts the scroll inset correctly alongside the custom mt-20 offset.
… with dynamic offset
LargeTitleHeader.ios.tsx renders <Stack.Screen options={propsToScreenOptions(...)}/>
on every render. propsToScreenOptions builds a new headerLargeStyle object each
time, and React Navigation's deep-equal misses the new reference, triggering a
navigation-state update which re-renders the screen, which re-renders the header,
which triggers another navigation-state update — an infinite loop that freezes
the app.
Fix: use contentInsetAdjustmentBehavior="never" to break the cycle. Since
LargeTitleHeader occupies no space in the React layout (it configures the native
nav controller), manually offset the ScrollView by safeAreaTop + 44 (compact
nav bar) + 52 (large-title row) using useSafeAreaInsets() so the content clears
the header correctly on all device sizes.
…entBehavior automatic Remove manual safe-area offset calculation; iOS handles header clearance correctly via contentInsetAdjustmentBehavior="automatic" on this screen.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@apps/expo/features/weather/hooks/useWeatherAlert.ts`:
- Around line 246-248: When locationId becomes falsy or a fetch errors the hook
currently leaves prior alerts visible; update the early-return branch that
checks locationId (and the error branch around lines 275–281) to explicitly
clear the alerts state before returning — e.g., call the hook's alert-state
setter to empty the list (setAlerts([]) or setWeatherAlerts([]) depending on the
existing state variable) and then setLoading(false) and return, and do the same
inside the fetch catch/failure path so stale alerts are never shown for the
wrong location.
- Around line 241-293: Refactor the hook useWeatherAlert to replace manual
useState/useEffect logic with TanStack React Query: create a useQuery keyed by
locationId (only enabled when locationId is truthy) that calls
getWeatherData(locationId), use the query's status/error/data to derive loading
and error, and use the query's select option (or map the data) to call
generateAlerts({ data: data as WeatherApiData, activeLocation: { name:
locationName } }) so setAlerts is no longer needed; ensure Sentry
breadcrumb/exception reporting is invoked inside the queryFn/ onError handlers
and return alerts, isLoading, and error from the hook.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 1d9f7185-382e-44db-873e-d9334f09048a
📒 Files selected for processing (4)
apps/expo/app/(app)/_layout.tsxapps/expo/app/(app)/weather-alerts.tsxapps/expo/features/weather/components/WeatherAlertsTile.tsxapps/expo/features/weather/hooks/useWeatherAlert.ts
Comment on lines
241
to
293
| const [alerts, setAlerts] = useState<WeatherAlert[]>([]); | ||
| const [loading, setLoading] = useState(true); | ||
| const [error, setError] = useState<string | null>(null); | ||
|
|
||
| useEffect(() => { | ||
| if (!activeLocation?.id) { | ||
| if (!locationId) { | ||
| setLoading(false); | ||
| return; | ||
| } | ||
|
|
||
| const locationId = activeLocation.id; | ||
| let cancelled = false; | ||
|
|
||
| async function fetchAlerts() { | ||
| setLoading(true); | ||
| setError(null); | ||
|
|
||
| Sentry.addBreadcrumb({ | ||
| category: 'weather', | ||
| message: 'Fetching weather alerts', | ||
| level: 'info', | ||
| data: { locationId, locationName }, | ||
| }); | ||
|
|
||
| try { | ||
| const data = await getWeatherData(locationId); | ||
| if (cancelled) return; | ||
|
|
||
| const formatted = generateAlerts({ | ||
| // safe-cast: getWeatherData returns WeatherApiForecastResponse; WeatherApiData is a structural subset used only by this alert generator. | ||
| data: data as unknown as WeatherApiData, | ||
| activeLocation, | ||
| activeLocation: { name: locationName }, | ||
| }); | ||
| setAlerts(formatted); | ||
| } catch (err) { | ||
| if (cancelled) return; | ||
| console.error('Weather alerts error:', err); | ||
| setError('Failed to fetch weather alerts'); | ||
| Sentry.captureException(err, { | ||
| tags: { feature: 'weatherAlerts', action: 'fetchAlerts' }, | ||
| extra: { locationId, locationName }, | ||
| }); | ||
| } finally { | ||
| setLoading(false); | ||
| if (!cancelled) setLoading(false); | ||
| } | ||
| } | ||
|
|
||
| fetchAlerts(); | ||
| }, [activeLocation]); | ||
|
|
||
| return () => { | ||
| cancelled = true; | ||
| }; | ||
| }, [locationId, locationName]); | ||
|
|
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify this hook is using manual server-state management instead of TanStack Query.
fd -a "useWeatherAlert.ts" | xargs -I{} rg -n "useQuery|`@tanstack/react-query`|useEffect\\(|useState\\(|getWeatherData\\(" "{}"Repository: PackRat-AI/PackRat
Length of output: 197
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate the file and print the relevant section with line numbers
FILE="apps/expo/features/weather/hooks/useWeatherAlert.ts"
if [ ! -f "$FILE" ]; then
echo "Missing: $FILE" >&2
exit 1
fi
echo "=== Imports (first 120 lines) ==="
sed -n '1,120p' "$FILE" | nl -ba
echo "=== Hook implementation (220-320) ==="
sed -n '220,320p' "$FILE" | nl -ba
echo "=== Search within file for react-query usage ==="
rg -n "`@tanstack/react-query`|useQuery|useMutation|useInfiniteQuery" "$FILE" || true
echo "=== Search within file for manual fetch lifecycle ==="
rg -n "useEffect\\(|useState\\(|getWeatherData\\(|cancelled|setLoading\\(|setError\\(" "$FILE"Repository: PackRat-AI/PackRat
Length of output: 158
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate the file and print the relevant section with line numbers
FILE="apps/expo/features/weather/hooks/useWeatherAlert.ts"
if [ ! -f "$FILE" ]; then
echo "Missing: $FILE" >&2
exit 1
fi
echo "=== Imports (first 120 lines) ==="
sed -n '1,120p' "$FILE" | nl -ba
echo "=== Hook implementation (220-320) ==="
sed -n '220,320p' "$FILE" | nl -ba
echo "=== Search within file for react-query usage ==="
rg -n "`@tanstack/react-query`|useQuery|useMutation|useInfiniteQuery" "$FILE" || true
echo "=== Search within file for manual fetch lifecycle ==="
rg -n "useEffect\\(|useState\\(|getWeatherData\\(|cancelled|setLoading\\(|setError\\(" "$FILE"Repository: PackRat-AI/PackRat
Length of output: 158
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="apps/expo/features/weather/hooks/useWeatherAlert.ts"
test -f "$FILE" || { echo "Missing: $FILE" >&2; exit 1; }
echo "=== Imports (first 140 lines) ==="
sed -n '1,140p' "$FILE" | cat -n
echo "=== Hook implementation (200-330) ==="
sed -n '200,330p' "$FILE" | cat -n
echo "=== react-query usage in file ==="
rg -n "`@tanstack/react-query`|useQuery|useMutation|useInfiniteQuery" "$FILE" || true
echo "=== manual lifecycle usage in file ==="
rg -n "useEffect\\(|useState\\(|getWeatherData\\(|cancelled|setLoading\\(|setError\\(" "$FILE"Repository: PackRat-AI/PackRat
Length of output: 8565
Refactor useWeatherAlerts to use TanStack React Query for server-state lifecycle
In apps/expo/features/weather/hooks/useWeatherAlert.ts (lines 241-293), useWeatherAlerts manually manages server state with useState/useEffect + a cancelled flag while calling getWeatherData. Replace this with useQuery keyed by locationId (enabled only when locationId exists) and derive alerts, loading, and error from the query result (optionally select → generateAlerts(...)).
🤖 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 `@apps/expo/features/weather/hooks/useWeatherAlert.ts` around lines 241 - 293,
Refactor the hook useWeatherAlert to replace manual useState/useEffect logic
with TanStack React Query: create a useQuery keyed by locationId (only enabled
when locationId is truthy) that calls getWeatherData(locationId), use the
query's status/error/data to derive loading and error, and use the query's
select option (or map the data) to call generateAlerts({ data: data as
WeatherApiData, activeLocation: { name: locationName } }) so setAlerts is no
longer needed; ensure Sentry breadcrumb/exception reporting is invoked inside
the queryFn/ onError handlers and return alerts, isLoading, and error from the
hook.
Comment on lines
+246
to
248
| if (!locationId) { | ||
| setLoading(false); | ||
| return; |
Contributor
There was a problem hiding this comment.
Clear stale alerts when location is missing or fetch fails.
The hook can keep showing previous-location alerts when locationId becomes falsy or when a fetch errors.
That exposes incorrect alert data to users.
Suggested fix
useEffect(() => {
if (!locationId) {
+ setAlerts([]);
+ setError(null);
setLoading(false);
return;
}
@@
} catch (err) {
if (cancelled) return;
console.error('Weather alerts error:', err);
+ setAlerts([]);
setError('Failed to fetch weather alerts');
Sentry.captureException(err, {
tags: { feature: 'weatherAlerts', action: 'fetchAlerts' },
extra: { locationId, locationName },
});Also applies to: 275-281
🤖 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 `@apps/expo/features/weather/hooks/useWeatherAlert.ts` around lines 246 - 248,
When locationId becomes falsy or a fetch errors the hook currently leaves prior
alerts visible; update the early-return branch that checks locationId (and the
error branch around lines 275–281) to explicitly clear the alerts state before
returning — e.g., call the hook's alert-state setter to empty the list
(setAlerts([]) or setWeatherAlerts([]) depending on the existing state variable)
and then setLoading(false) and return, and do the same inside the fetch
catch/failure path so stale alerts are never shown for the wrong location.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes #2498 — screen freeze when tapping Weather Alerts on iOS.
activeLocationAtomderives its value via.find(), emitting a new object reference on every evaluation. Passing the wholeactiveLocationobject as auseEffectdependency causeduseWeatherAlertsto re-fetch on every render, triggering a cascade that could freeze the UI.locationIdandlocationNamefrom the atom value and use those as stable effect dependencies instead of the object reference.Alertcomponent fromWeatherAlertsTilethat was rendering new object/array literals (materialIcon,buttons) on every render — NativeWindUI's internal effects looped on these unstable props.weather-alertsscreenanimationfromslide_from_bottom(modal idiom) todefaultto match itscardpresentation.cancelledflag) and Sentry instrumentation touseWeatherAlerts.Test plan
Summary by CodeRabbit
Bug Fixes
Improvements