ref(android): Bound Nav3 argument payload size + report argument drop reasons - #6218
0xadam-brown wants to merge 2 commits into
Conversation
… reasons Commit makes three refinements to how our Nav3 integration handles host-app provided arguments: 1. Limits the total argument characters sanitized from one back-stack update so large strings and stringified values cannot dominate navigation telemetry payloads or processing time. 2. Attaches a dropped reason marker when arguments are omitted for performance or safety reasons. 3. Reduces some of our argument limits in response to benchmark testing results.
📲 Install BuildsAndroid
|
0xadam-brown
added
the
sanity-check
…k stack context + drop visually redundant "arguments_" prefix from drop reason
![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 f5563cb. Configure here.
| if (arguments.isNotEmpty()) { | ||
| put("arguments", arguments) | ||
| } | ||
| argumentsWithMetadata().takeIf { it.isNotEmpty() }?.let { put("entry_arguments", it) } |
There was a problem hiding this comment.
Serialization key mismatch: "entry_arguments" vs "arguments"
High Severity
The serialize() method now uses "entry_arguments" as the map key, but every test — both existing ones (line 622) and newly added ones (line 640) — still expects "arguments". The same mismatch exists in BackStackObserverTest at lines 276, 279, 464, 700, and 735. Either the production key or all the test expectations are wrong, meaning either tests will fail or the emitted navigation telemetry uses an unintended key name that downstream consumers (Sentry backend/UI) won't recognize.
Additional Locations (2)
Reviewed by Cursor Bugbot for commit f5563cb. Configure here.
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
| NormalizedSentryBackStackEntry( | ||
| name = "/HomeScreen", | ||
| argumentDropReason = ArgumentDropReason.MAX_COUNT, | ||
| ) | ||
|
|
||
| assertThat(entry.serialize()) | ||
| .isEqualTo( | ||
| mapOf( | ||
| "entry" to "/HomeScreen", | ||
| "arguments" to mapOf(ARGUMENT_DROP_REASON_KEY to "max_argument_count_exceeded"), | ||
| ) |
There was a problem hiding this comment.
Bug: Unit tests in BackStackConverterTest.kt use an outdated serialization key "arguments" in assertions, while the production code now uses "entry_arguments", causing test failures.
Severity: LOW
Suggested Fix
Update the assertions in the failing tests within BackStackConverterTest.kt. The expected maps in the assertThat calls should be modified to use the key "entry_arguments" instead of "arguments" to align with the updated serialization logic.
Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent. Verify if this is a real issue. If it is, propose a fix; if not, explain why it's
not valid.
Location:
sentry-android-navigation3/src/test/kotlin/io/sentry/compose/navigation3/BackStackConverterTest.kt#L629-L641
Potential issue: The production code in `BackStackConverter.kt` was updated to serialize
back stack entry arguments with the key `entry_arguments`. However, unit tests in
`BackStackConverterTest.kt` were not updated to reflect this change. The tests
`serialize returns entries in serialized form` and `serialize includes the reason
arguments were dropped` still contain assertions that expect the old key, `arguments`.
This discrepancy will cause the tests to fail, preventing the pull request from being
merged if CI enforces test passage.
Also affects:
sentry-android-navigation3/src/test/kotlin/io/sentry/compose/navigation3/BackStackConverterTest.kt:624
Did we get this right? 👍 / 👎 to inform future reviews.
This branch has not been deployed
1 participant
📜 Description
PR makes three refinements to how our Nav3 integration handles host-app provided arguments:
Limits the total argument characters sanitized from one back-stack update so large strings and stringified values cannot dominate navigation telemetry payloads or processing time.
Attaches a dropped reason marker when arguments are omitted for performance or safety reasons.
Reduces some of our argument limits in response to benchmark testing results.
💡 Motivation and Context
Addresses @markushi's helpful comment here + lets us incorporate findings from sample app benchmark testing.
addresses: JAVA-274
Screenshots
Back stack context
Breadcrumb
Span arguments
💚 How did you test it?
Unit tests.
📝 Checklist
sendDefaultPIIis enabled.🔮 Next steps