feat(android-nav3): [Android Nav3 2] Model navigation routes - #6130
0xadam-brown wants to merge 1 commit into
Conversation
|
📲 Install BuildsAndroid
|
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 061c4c3. Configure here.
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
e5d13ee to
1b98b50
Compare
7ea1f4c to
dc5546c
Compare
Introduce the route translation layer for Navigation 3 so later stages can reason about route names and sanitized arguments in one place. At this stage the module still exposes no meaningful public API surface.
dc5546c to
b0eb957
Compare
| * Doing so prevents route names from being obfuscated while leaving per-route arguments to | ||
| * [RouteArgumentsExtractor]. | ||
| */ | ||
| @ApiStatus.Experimental |
There was a problem hiding this comment.
Marked as experimental b/c this will be made public in a follow-on to the current PR stack. (Same below.)
0xadam-brown
added
the
deep-dive
runningcode
left a comment
runningcode
left a comment
There was a problem hiding this comment.
before I give it a deeper look or maybe we can answer these in our session later, since I'm not a compose expert, what's the threading behavior on this? are these always called from the main thread since this is UI stuff?
One more question is that I wonder if it is safe to call the logger from inside the catch clause. I know we've had situations in the past where this was unsafe due to the SO/recursion risk.
The issue is that the logger also has user callbacks in the form of BeforeSendLogCallback and is therefore unsafe in the same way that the user callbacks are.
|
|
||
| testImplementation(libs.androidx.compose.runtime) | ||
| testImplementation(libs.google.truth) | ||
| testImplementation(libs.kotlin.test.junit) |
There was a problem hiding this comment.
do we need this? or can we just use google truth?
| import io.sentry.compose.navigation3.RouteTranslator.WarningState | ||
| import java.util.AbstractCollection | ||
| import kotlin.test.Test | ||
| import kotlin.test.assertEquals |
There was a problem hiding this comment.
can we use google truth on this one ?
| import androidx.compose.runtime.snapshots.Snapshot | ||
| import com.google.common.truth.Truth.assertThat | ||
| import kotlin.test.Test | ||
| import kotlin.test.assertEquals |
There was a problem hiding this comment.
can we use truth here?
markushi
left a comment
markushi
left a comment
There was a problem hiding this comment.
Looking good, left two minor commments.
| * enforced across the whole update. Once the budget is spent, the overflowing entry and every | ||
| * older entry are dropped, while newer (already-processed) entries are preserved. | ||
| */ | ||
| internal class ArgumentSanitizer( |
There was a problem hiding this comment.
Nice approach! I'm wondering if we should indicate that the resulting maps is incomplete due to an extraction error or budget exhaustion. Could be as simple as adding a synthetic entry ~ "sentry_arguments_truncated": true
| * Extractor invocations are hidden from Compose snapshot observation so they don't impact | ||
| * invalidation of the recompose scope that reads them. | ||
| */ | ||
| internal class RouteResolvers<T : Any>( |
There was a problem hiding this comment.
Any chance we could unify the naming/terms a bit here? I read 'resolver', 'extractor' and 'translator' and they all sound too similar to my non-native brain 🙈 Maybe 'resolver' and 'extractor' could be merged, e.g. RouteResolvers -> RouteExtractors and keep RouteNameExtractor+ RouteArgumentsExtractor as-is.
3 participants
PR Stack (Android Nav3)
📜 Description
To support Nav3 we have to map a host app's back stack to an internal representation suitable for the Sentry data we emit. This PR introduces the core logic for doing so in the form of a
RouteTranslator.Note that the
RouteResolverspassed to the translators constructor are defined by the host app, and so we're careful to protect against misuse / unintended performance issues.💡 Motivation and Context
addresses: JAVA-274
💚 How did you test it?
📝 Checklist
sendDefaultPIIis enabled.🔮 Next steps
Build the observer layer that consumes these routes and updates Sentry state.
#skip-changelog