Repository navigation
fix(tracing): Name transaction when re-navigating to focused route with new params - #6606
Merged
Merged
Conversation
…th new params Navigating to the already-focused route with only params changed keeps the same route.key. The state-change handler treated this as a no-op and dropped the navigation span, which then self-finalized as an unnamed "Route Change". Distinguish same-key/params-changed re-navigation (name and finalize) from a true no-op same-route change (keep the idle path) via a shallow param compare. Fixes #6593 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Contributor
Semver Impact of This PR⚪ None (no version bump detected) 📋 Changelog PreviewThis is how your changes will appear in the changelog.
🤖 This preview updates automatically when you update the PR. |
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
📲 Install BuildsAndroid
|
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit b8b3052. Configure here.
![sentry-warden[bot]](https://avatars.githubusercontent.com/in/2755838?s=60&v=4){kind=small}
Contributor
Android (legacy) Performance metrics 🚀
|
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| f9c1ed4+dirty | 431.00 ms | 466.22 ms | 35.22 ms |
| c2e182c+dirty | 471.64 ms | 553.59 ms | 81.95 ms |
| 41d6254+dirty | 424.45 ms | 474.34 ms | 49.89 ms |
| af33f3b+dirty | 434.90 ms | 506.14 ms | 71.24 ms |
| 882f8ae+dirty | 399.98 ms | 427.06 ms | 27.08 ms |
| 0b5120f+dirty | 503.22 ms | 538.60 ms | 35.38 ms |
| 774257e+dirty | 407.09 ms | 447.46 ms | 40.37 ms |
| bf168a4+dirty | 418.21 ms | 489.74 ms | 71.53 ms |
| 5569641+dirty | 406.43 ms | 428.51 ms | 22.08 ms |
| c151573+dirty | 530.34 ms | 559.43 ms | 29.09 ms |
App size
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| f9c1ed4+dirty | 49.74 MiB | 54.86 MiB | 5.12 MiB |
| c2e182c+dirty | 49.74 MiB | 54.85 MiB | 5.11 MiB |
| 41d6254+dirty | 48.30 MiB | 53.60 MiB | 5.30 MiB |
| af33f3b+dirty | 49.74 MiB | 55.09 MiB | 5.35 MiB |
| 882f8ae+dirty | 48.30 MiB | 53.60 MiB | 5.29 MiB |
| 0b5120f+dirty | 48.30 MiB | 53.58 MiB | 5.28 MiB |
| 774257e+dirty | 48.30 MiB | 53.58 MiB | 5.28 MiB |
| bf168a4+dirty | 49.74 MiB | 55.09 MiB | 5.35 MiB |
| 5569641+dirty | 48.30 MiB | 53.48 MiB | 5.18 MiB |
| c151573+dirty | 48.30 MiB | 53.54 MiB | 5.24 MiB |
… test Address Warden review on #6606: - The unchanged-params same-key branch now clears the route-change timeout and ends the processing span, so a leftover timer cannot later discard the next navigation's span. - Assert the empty default-config re-nav ships no named "Chat" transaction, so the intentional empty-back-navigation discard is pinned explicitly. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit d7615c4. Configure here.
Contributor
iOS (legacy) Performance metrics 🚀
|
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| 71abba0+dirty | 3821.93 ms | 1202.81 ms | -2619.12 ms |
| bc0d8cf+dirty | 3830.33 ms | 1220.52 ms | -2609.81 ms |
| 68ae91b+dirty | 3834.04 ms | 1216.60 ms | -2617.44 ms |
| bf168a4+dirty | 3838.60 ms | 1218.98 ms | -2619.62 ms |
| 267d3ed+dirty | 3853.65 ms | 1224.70 ms | -2628.94 ms |
| f170ec3+dirty | 3822.26 ms | 1218.33 ms | -2603.93 ms |
| 84cba31+dirty | 3846.74 ms | 1224.80 ms | -2621.93 ms |
| b9bebee+dirty | 3850.15 ms | 1227.51 ms | -2622.64 ms |
| d038a14+dirty | 3845.71 ms | 1228.11 ms | -2617.59 ms |
| 853723c+dirty | 3852.60 ms | 1234.64 ms | -2617.96 ms |
App size
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| 71abba0+dirty | 5.15 MiB | 6.67 MiB | 1.52 MiB |
| bc0d8cf+dirty | 5.15 MiB | 6.67 MiB | 1.51 MiB |
| 68ae91b+dirty | 4.98 MiB | 6.46 MiB | 1.48 MiB |
| bf168a4+dirty | 4.98 MiB | 6.47 MiB | 1.49 MiB |
| 267d3ed+dirty | 5.15 MiB | 6.69 MiB | 1.54 MiB |
| f170ec3+dirty | 5.15 MiB | 6.69 MiB | 1.53 MiB |
| 84cba31+dirty | 4.98 MiB | 6.55 MiB | 1.57 MiB |
| b9bebee+dirty | 5.15 MiB | 6.68 MiB | 1.53 MiB |
| d038a14+dirty | 5.15 MiB | 6.67 MiB | 1.51 MiB |
| 853723c+dirty | 5.15 MiB | 6.69 MiB | 1.53 MiB |
Contributor
iOS (new) Performance metrics 🚀
|
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| 7a89652+dirty | 3827.11 ms | 1211.49 ms | -2615.62 ms |
| 0a147b2+dirty | 3845.11 ms | 1230.81 ms | -2614.30 ms |
| 5b7e8a7+dirty | 3855.46 ms | 1234.47 ms | -2620.99 ms |
| 5fe1c6c+dirty | 1201.36 ms | 1209.15 ms | 7.78 ms |
| 95c2f89+dirty | 3849.40 ms | 1226.85 ms | -2622.54 ms |
| 580fb5c+dirty | 3835.45 ms | 1218.61 ms | -2616.84 ms |
| 94af3bd+dirty | 3847.84 ms | 1230.33 ms | -2617.51 ms |
| f3215d3+dirty | 3846.08 ms | 1231.85 ms | -2614.23 ms |
| a636fa4+dirty | 3846.16 ms | 1210.10 ms | -2636.05 ms |
| 5a21b51+dirty | 3837.87 ms | 1223.47 ms | -2614.40 ms |
App size
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| 7a89652+dirty | 5.15 MiB | 6.70 MiB | 1.55 MiB |
| 0a147b2+dirty | 4.98 MiB | 6.51 MiB | 1.53 MiB |
| 5b7e8a7+dirty | 5.15 MiB | 6.70 MiB | 1.54 MiB |
| 5fe1c6c+dirty | 3.38 MiB | 4.77 MiB | 1.39 MiB |
| 95c2f89+dirty | 5.08 MiB | 6.75 MiB | 1.67 MiB |
| 580fb5c+dirty | 4.98 MiB | 6.46 MiB | 1.48 MiB |
| 94af3bd+dirty | 5.15 MiB | 6.69 MiB | 1.53 MiB |
| f3215d3+dirty | 5.15 MiB | 6.67 MiB | 1.52 MiB |
| a636fa4+dirty | 4.98 MiB | 6.55 MiB | 1.57 MiB |
| 5a21b51+dirty | 5.15 MiB | 6.67 MiB | 1.51 MiB |
…te compare
haveRouteParamsChanged walked only a's keys, so equal-count bags with disjoint
keys whose values are undefined (e.g. { a: undefined } vs { b: undefined })
compared equal. Require each key to exist on b too. Addresses find-bugs review.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
oxlint no-unnecessary-type-assertion flagged the casts; b ?? {} already
yields Record<string, unknown>. Bind aBag/bBag via nullish coalescing instead.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…arams-navigation # Conflicts: # CHANGELOG.md
…6593) Add two cases flagged by review: - useDispatchedActionData off, same-key changed params: proves the fix names the transaction instead of leaking an unnamed "Route Change" (the actual bug mode; verified to fail on the pre-fix branch). - same-key unchanged params: locks the no-op idle path (no route.key/route.name attributes set), guarding the timeout-clear + processing-span-end branch. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Contributor
Android (new) Performance metrics 🚀
|
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| 7436d0f+dirty | 429.58 ms | 452.52 ms | 22.94 ms |
| 1122a96+dirty | 510.16 ms | 542.00 ms | 31.84 ms |
| 26843eb+dirty | 442.02 ms | 506.29 ms | 64.27 ms |
| 88735e9+dirty | 427.04 ms | 487.37 ms | 60.33 ms |
| 882f8ae+dirty | 408.19 ms | 435.86 ms | 27.67 ms |
| 1a2e7e0+dirty | 451.98 ms | 501.50 ms | 49.52 ms |
| 68672fc+dirty | 407.55 ms | 442.96 ms | 35.41 ms |
| 4966363+dirty | 415.67 ms | 448.60 ms | 32.93 ms |
| 038a6d7+dirty | 499.02 ms | 527.68 ms | 28.66 ms |
| 6176a94+dirty | 403.58 ms | 446.73 ms | 43.15 ms |
App size
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| 7436d0f+dirty | 48.30 MiB | 53.60 MiB | 5.30 MiB |
| 1122a96+dirty | 48.30 MiB | 53.54 MiB | 5.24 MiB |
| 26843eb+dirty | 49.74 MiB | 55.26 MiB | 5.52 MiB |
| 88735e9+dirty | 49.74 MiB | 54.82 MiB | 5.07 MiB |
| 882f8ae+dirty | 48.30 MiB | 53.60 MiB | 5.29 MiB |
| 1a2e7e0+dirty | 49.74 MiB | 54.82 MiB | 5.07 MiB |
| 68672fc+dirty | 48.30 MiB | 53.61 MiB | 5.31 MiB |
| 4966363+dirty | 48.30 MiB | 53.54 MiB | 5.24 MiB |
| 038a6d7+dirty | 48.30 MiB | 53.60 MiB | 5.30 MiB |
| 6176a94+dirty | 48.30 MiB | 53.54 MiB | 5.24 MiB |
antonis
marked this pull request as ready for review
August 24, 2026 14:57
Contributor
| // Consume any pending deep link and attach it to this span. Done here | ||
| // (after route info is known) so the link is only attributed to a span | ||
| // that actually mounted a route — not one that was later discarded. | ||
| applyPendingDeepLinkToSpan(latestNavigationSpan, routeChangeTimeoutMs); |
Collaborator
There was a problem hiding this comment.
doing some extra checks, why we apply applyPendingDeepLinkToSpan here and also on line 724 on the same function? Unrelated to the PR so this can be fixed on a follow up PR
Contributor
Author
There was a problem hiding this comment.
I'll recheck the logic introduced on #6264 and iterate on this. Thank you for raising 🙇
Contributor
Author
There was a problem hiding this comment.
I rechecked this and I think both calls are needed and cover different branches. Line 724 (inside the same-route-key block) is required so a deep link to the current screen tags the span before the POP_TO discard check on 737. In that branch line 799 is never reached. Line 799 is the only call that runs for a normal route change. Let me know if that is not the case 🙇
lucas-zimerman
approved these changes
Aug 25, 2026
lucas-zimerman
left a comment
lucas-zimerman
left a comment
Collaborator
There was a problem hiding this comment.
Overall looks good! left a comment to an unrelated change. LGTM!
3 tasks
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
2 participants
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.
📢 Type of change
📜 Description
Navigating to the route you're already on with only the params changed — e.g.
navigate('Chat', { id: 2 })whileChat/id:1is focused — keeps the sameroute.key. Thestate-change handler treated every same-key change as ano-op: it dropped its reference to the navigation span without naming or
finalizing it. The idle span then self-finalized and shipped as an unnamed
"Route Change", with a dangling
navigation.processingchild and anuncleared route-change timeout.
The same-key branch now distinguishes two cases via a shallow param comparison
(
haveRouteParamsChanged):SET_PARAMS, deep link to thecurrent screen) — unchanged: keep the idle-path behaviour.
the transaction ships named with
route.name/route.key/route.has_been_seen, endsnavigation.processing, and clears the timeout.The change is confined entirely to the
previousRoute.key === route.keybranch; different-key navigation and same-key no-param-change changes are
unaffected.
💡 Motivation and Context
Fixes #6593
💚 How did you test it?
Added unit tests in
reactnavigation.test.tscovering a params-changedre-navigation onto the focused route (ships a named
Chattransaction with theright route attributes, no unnamed "Route Change") and the empty-re-nav case
under default config. Full suite passes (123/123). Existing same-route tests
(drawer /
SET_PARAMS/POP_TO/ deep-link attribution) remain green.📝 Checklist
sendDefaultPIIis enabled.🔮 Next steps