Sign up
View stargazers13Sign up

fix(mobile): a closed search no longer risks freezing the app#23148

merged2
duo/search
duo/search-observer
opened3d agobyJJulius Marminge
5 files changed
View on GitHubTwo-way sync enabledPR last imported 3d ago

UIKit's search field leaves a main-queue observer on its text storage after the field is freed: _UISearchBarTextFieldTokenCounter registers for NSTextStorageDidProcessEditingNotification, filtered by the storage's pointer. When React Native's JS thread later measures text in a new storage at the same address, the edit waits for the main thread while holding React Native's text measurement lock. A fold that measures text on the main thread then deadlocks. Closing any screen with a search, or re-placing a search on a fold, left one behind. This was reproduced on the Duo simulator with no XCTest loaded.

  • Screens 5.0.0-t3.33 removes those observers when their storage is freed.
  • Screens 5.0.0-t3.34 narrows that to UIKit's own search field observer, identified by the block function UIKit registers while the first search controller is created, and holds it weakly.

Warning

Needs a deliberate review. To recognize UIKit's registration, the fork replaces -[NSNotificationCenter addObserverForName:object:queue:usingBlock:] process-wide (from +[RNSSearchBar initialize]), and it relies on the shape of a private UIKit block. Treat it as a workaround to remove once UIKit or React Native fixes the underlying issue, not as a final fix. Sources: t3.33 and t3.34, in pingdotgg/react-native-screens#3.

Verification

lldb on the t3.34 dev client counted the remover's work across opening and closing searches and folding:

  • 3 UIKit token counters were created, 3 removers attached, and 3 observers were removed.
  • A third-party text storage observer, registered as a control, kept firing and was never touched.

There are no screenshots because the change has no visual effect.

Implemented with Claude Opus 5.5 in Claude Code, running in T3 Code.

🤖 Generated with Claude Code

Checks

All checks passed
Ggithub-actionsBot3d ago
Synced from GitHub

Thread transfer impact

✅ Thread transfer remains within every enforced ceiling.

ℹ️ No successful main baseline artifact is available yet. This run establishes the initial measurement.

ProviderMetricMain baselineThis PRImpactPR ceiling
CodexTotal thread wire—5.0 KiB—6.8 KiB✅
CodexThread snapshot wire—3.8 KiB—4.9 KiB✅
CodexLive turn WebSocket wire—1.2 KiB—2.0 KiB✅
CodexLive turn WebSocket decoded—20.9 KiB—29.3 KiB✅
CodexLive turn messages—2—8✅
ClaudeTotal thread wire—5.0 KiB—6.8 KiB✅
ClaudeThread snapshot wire—3.8 KiB—4.9 KiB✅
ClaudeLive turn WebSocket wire—1.2 KiB—2.0 KiB✅
ClaudeLive turn WebSocket decoded—21.2 KiB—29.3 KiB✅
ClaudeLive turn messages—2—8✅

Baseline: unavailable · PR result: fdfc6ea · Source CI: success

Scenario and decoded snapshot size

10 historical turns, 5 command tools per turn, 878.9 KiB retained MCP result per historical turn, and a 1.05 MiB retained result in the measured turn.

  • Codex decoded thread snapshot: 108.5 KiB
  • Claude decoded thread snapshot: 108.8 KiB

Updated in place by a trusted workflow. PR artifacts are strictly validated and never executed.

MmacroscopeappBot3d ago
Synced from GitHub

Approvability

Verdict: Needs Human Approval

Macroscope's review found this PR requires human approval — This updates the production iOS Screens dependency with a process-wide notification-registration hook that relies on private UIKit implementation details to prevent stale search observers. The targeted freeze fix is plausible, but the native interception and compatibility risks warrant deliberate human review.

No code changes detected at fdfc6ea. Prior analysis still applies.

You can add or adjust custom eligibility rules. Learn more.

CcoderabbitaiBot3d ago
Synced from GitHub

Review in Change Stack →

Important

Review skipped

We couldn't safely recover the incremental review. No full review was started, and the last reviewed checkpoint was preserved. Retry later, or explicitly request a full review by commenting @coderabbitai full review.

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration
  • Configuration used: Path: .coderabbit.config.ts
  • Review profile: CHILL
  • Plan: Team
  • Run ID: 0a7b6b47-4397-4d45-bda3-e446eb4d1dc8
📥 Commits

Reviewing files that changed from the base of the PR and between 202d4a9b2cc0e3fe047bcb2390cf6ec9404a6440 and 902e90705352ea20f8d6219441153011fb58df4e.

⛔ Files ignored due to path filters: 1
  • pnpm-lock.yaml is excluded by !**/pnpm-lock.yaml
📒 Files selected for processing: 4
  • apps/mobile/deps/react-native-screens-5.0.0-t3.32.tgz
  • apps/mobile/deps/react-native-screens-5.0.0-t3.34.tgz
  • apps/mobile/package.json
  • docs/operations/mobile-screens-fork.md

Limit details: You’ve used all 10 included reviews currently available.

📝 Walkthrough

The mobile app now references the react-native-screens t3.34 archive. The operations documentation now links to the matching t3.34 fork reference.

Changes: Mobile Screens Fork Update

Layer / File(s)Summary
Update Screens fork version
apps/mobile/package.json, docs/operations/mobile-screens-fork.md		The mobile dependency archive and documented fork link now reference t3.34.

Priority: ➖ Normal

Estimated code review effort: 1 (Trivial) | ~3 minutes

Change: Bug fix

Suggested reviewers: yash-singh1

Merge Risk: ⚪ Minimal · up to 902e9

The dependency update is consistent across the manifest and documentation, with no concrete merge risk identified.

Pre-merge checks: Passed 4
Check nameStatusExplanation
Title checkPassedThe title clearly describes the primary change: preventing app freezes caused by closing screens with search fields.
Description checkPassedThe description explains the freeze, the fork update, the private UIKit workaround, and focused LLDB verification. It is mostly complete, but it does not use explicit Problem, Change, or Scope and app…
Linked Issues checkPassedCheck skipped because no linked issues were found for this pull request.
Out of Scope Changes checkPassedCheck skipped because no linked issues were found for this pull request.
Full details: Description check

Explanation

The description explains the freeze, the fork update, the private UIKit workaround, and focused LLDB verification. It is mostly complete, but it does not use explicit Problem, Change, or Scope and approval headings and does not provide explicit maintainer approval or a clear exemption rationale.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Commit to this branch
  • Create a new PR
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Comment @coderabbitai help to get the list of available commands.

Added 2 commits

  • fix(mobile): a closed search no longer risks freezing the app5bba602
  • fix(mobile): the search freeze fix touches only UIKit's search field observerfdfc6ea