fix(mobile): harden native header toolbar items - #8611
Conversation
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Comment |
Thread transfer impact✅ Thread transfer remains within every enforced ceiling.
Baseline: Scenario and decoded snapshot size10 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.
Updated in place by a trusted workflow. PR artifacts are strictly validated and never executed. |
93a9845 to
820f38d
Compare
| @@ -738,7 +754,8 @@ index 1c844846a5c66e31cfa530ca462774d2fb6bbea1..d36bda28552e5ac75088e18d0fde338e | |||
| + if (!reuseToolbarItems) { | |||
There was a problem hiding this comment.
🟡 Medium patches/react-native-screens@4.26.2.patch:754
A prop refresh on a non-top vc replaces its toolbarItems and calls setToolbarHidden:, so a background screen can hide or show the toolbar belonging to the active pushed screen. reuseToolbarItems is deliberately false for non-top controllers, but the subsequent if still applies the update; restrict this block to vc being navctr.topViewController.
| + if (!reuseToolbarItems) { | |
| if (!reuseToolbarItems && navctr.topViewController == vc) { |
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @patches/react-native-screens@4.26.2.patch around line 754:
A prop refresh on a non-top `vc` replaces its `toolbarItems` and calls `setToolbarHidden:`, so a background screen can hide or show the toolbar belonging to the active pushed screen. `reuseToolbarItems` is deliberately false for non-top controllers, but the subsequent `if` still applies the update; restrict this block to `vc` being `navctr.topViewController`.
ApprovabilityVerdict: Would Approve Macroscope's review found this PR approvable — This is a localized mobile hardening fix for existing iOS header-toolbar and search event handling, with the lockfile only pinning the updated dependency patch. An unresolved medium-severity finding still identifies a possible non-top-screen toolbar update affecting the active screen. Not approved because:
Adjust the Minimum Blocking Severity for this repo — including turning it Off — in Settings. You can add or adjust custom eligibility rules. Learn more. |
820f38d to
c410830
Compare
The Expo 57 upgrade carried four correctness risks in our
react-native-screenstoolbar patch: placement ID collisions, invalid UIKit search-placement grouping, missing native search callbacks, and dropped mixed toolbar items. This follow-up addresses the findings from #8609.The patch now gives right, center, and toolbar items distinct event namespaces; filters UIKit search-placement items to the toolbar in both JS and native code; wires text changes to either the native or fallback search field without accumulating duplicate actions; and removes only the custom Mail search item when building the ordinary navigation toolbar.
Review findings addressed:
Verification:
vp run --filter @t3tools/mobile typecheckvp run lint:mobilevp i --frozen-lockfilevp exec expo install --checkfromapps/mobileT3CodeDeviOS Simulator build succeeded against the patched native sourceGenerated with GPT-5.6-sol in T3 Code using the Codex harness.
Note
Medium Risk
Changes touch iOS navigation bar/toolbar native code and JS event routing; incorrect wiring could break header buttons or search UX, but scope is limited to the vendored screens patch.
Overview
Updates the
react-native-screens@4.26.2patch (and lockfile patch hash) to fix four header/toolbar regressions called out after the Expo 57 upgrade.Event IDs: Center and toolbar bar items are now prepared with
centerandtoolbarplacements instead of reusingright, sobuttonId/menuIdvalues no longer collide across header regions.Search placement:
searchBarPlacementitems are dropped in JS unless the placement istoolbar, and nativebarButtonItemsFromConfigsskips them whenallowsSearchBarPlacementis false (header left/right/center); only the navigation toolbar path enables search placement.Mail search toolbar: Search text changes are wired on a shared
resolvedSearchTextField(native search bar or fallback field) with remove-then-add actions so edits do not stack duplicate handlers. MixedheaderToolbarItemslists keep non-mailSearchToolbarentries for the standard toolbar while the custom mail-search chrome still uses the dedicated config.pnpm-lock.yaml reflects the new patch digest for
react-native-screensand dependent resolution entries.Reviewed by Cursor Bugbot for commit c410830. Bugbot is set up for automated code reviews on this repo. Configure here.
Note
Harden native header toolbar items in mobile
Updates the pnpm lockfile as part of hardening native header toolbar items on mobile. The lockfile change reflects dependency updates supporting the toolbar fix.
Macroscope summarized c410830.