refactor(platform-android)!: report changed permission ids, not a joined string - #3023
Conversation
|
Size Report
Startup median (7 runs, lower is better):
|
There was a problem hiding this comment.
All reported issues were addressed across 3 files
Reply with feedback, questions, or to request a fix.
Fix all with cubic | Re-trigger cubic
|
Reviewed at 47f6d36. Named commands.md:764 says every Android response reports Not blocking: CI: Smoke Tests is still queued. No failures yet. |
…ed string permission still names the requested target; permissions: string[] carries the ids each revoke/all response actually mutated, replacing the comma-joined permission and the applied field. Error details use the same array instead of a joined string. Closes #2701
47f6d36 to
c55b03b
Compare
|
Pushed c55b03b (rebased on main).
CI: waiting on the new run; I will treat only the iOS Smoke Tests flake as acceptable. |
There was a problem hiding this comment.
1 issue found across 3 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="packages/platform-android/src/settings-permission.ts">
<violation number="1" location="packages/platform-android/src/settings-permission.ts:460">
P3: The newly added grant response is only asserted for `pm`-kind targets (location in the unit test, camera in the integration test). The `photos` and `notifications` grant branches also return a `{ permission, permissions }` response now — `[target.permission]` here and `[await setAndroidPhotoPermission(...)]` in the photos branch — and no test pins those. Add assertions on the returned `permission`/`permissions` for the existing `grant photos` and grant-notifications tests so the unified shape is actually covered for every kind.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Fix all with cubic | Re-trigger cubic
| ): Promise<string[]> { | ||
| if (target.kind === 'notifications') { | ||
| await setAndroidNotificationPermission(device, appPackage, 'grant', target, userArgs); | ||
| return [target.permission]; |
There was a problem hiding this comment.
P3: The newly added grant response is only asserted for pm-kind targets (location in the unit test, camera in the integration test). The photos and notifications grant branches also return a { permission, permissions } response now — [target.permission] here and [await setAndroidPhotoPermission(...)] in the photos branch — and no test pins those. Add assertions on the returned permission/permissions for the existing grant photos and grant-notifications tests so the unified shape is actually covered for every kind.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/platform-android/src/settings-permission.ts, line 460:
<comment>The newly added grant response is only asserted for `pm`-kind targets (location in the unit test, camera in the integration test). The `photos` and `notifications` grant branches also return a `{ permission, permissions }` response now — `[target.permission]` here and `[await setAndroidPhotoPermission(...)]` in the photos branch — and no test pins those. Add assertions on the returned `permission`/`permissions` for the existing `grant photos` and grant-notifications tests so the unified shape is actually covered for every kind.</comment>
<file context>
@@ -437,15 +454,18 @@ async function grantAndroidPermission(
+): Promise<string[]> {
if (target.kind === 'notifications') {
await setAndroidNotificationPermission(device, appPackage, 'grant', target, userArgs);
+ return [target.permission];
} else if (target.kind === 'photos') {
- await setAndroidPhotoPermission(device, appPackage, 'grant', userArgs);
</file context>
|
The PR is ready at c55b03b. The earlier findings from 47f6d36 are fixed. Not blocking: android-lifecycle.test.ts grows from 1259 to 1261 lines, which trips the Coverage size ratchet, so please keep it at 1259 or fewer by moving the new permission assertions into a smaller test file or making the edit net-zero, with no baseline entry. The photos and notifications grant tests in settings-permission.test.ts could also assert I did not run the tests. The 44/44 pass and the emulator output are author-reported. The live run used a Telegram package, not a coarse-only app, so only the unit test covers coarse-only behavior. Coverage fails because of this diff, for the reason above. Smoke Tests fails at "restore fixture home after WebView lab" (simctl openurl timed out, code=60). This PR touches only Android permission code, so that looks like the cold-toolchain iOS flake. I judged this from the step name and the file overlap, not the full log. No conflicts. Before merge, get android-lifecycle.test.ts to 1259 lines or fewer and re-run CI. Smoke Tests needs only a re-run. |
|
Pushed 076bcde. The camera grant in the Android lifecycle scenario now shares its fixed fields, so the test file is back under its size limit (1257 lines). Both camera-grant assertions are still there. |
|
This PR is ready at 076bcde. The earlier review at c55b03b was clean, and the change since then is test-only formatting, so the code result is the same. All checks pass. I did not run the tests locally, and the file has only one camera-grant call, so I read the two assertions on that call as the "both" assertions. Not blocking: the new docs line at https://github.com/callstack/agent-device/blob/076bcde/website/docs/docs/commands.md#L764 says |
Summary
Android
settings permissionresponses reported changed ids under two inconsistent shapes: a comma-joinedpermissionstring on named/pmtargets, and a separateappliedarray onall. Every response path now reportspermissionas the requested target name andpermissions: string[]as the ids actually mutated, in order.appliedfolds intopermissions; the not-requested error detail uses the array too. Updateswebsite/docs/docs/commands.mdaccordingly.3 files touched. No public TS response type exists for this command (return is
Record<string, unknown>), so no client-types change is needed; the repo has no CHANGELOG.md/changeset mechanism, so no changelog entry was added.Closes #2701. Nominally stacked on
refactor/settings-permission-typed-skip-2700(#2700), but that branch's diff already merged asa212d9a55a(#3010) and its remote branch is gone, so this branch is rebased directly ontomainand carries only the #2701 diff.Validation
Tested at
c55b03ba69.pnpm vitest run packages/platform-android/src/__tests__/settings-permission.test.ts test/integration/provider-scenarios/android-lifecycle.test.ts: 44/44 passed, including agrant locationcoarse-only test, a pinneddeny locationrevoke fan-out, and a provider scenario that assertspermissionandpermissionsend to end.pnpm check:affected --run:all runnable checks passed(onedaemon-entrypointtimeout from host contention on the first run; the rerun passed).org.telegram.messenger.web,settings permission grant locationthendeny location: