fix: don't re-request contacts permission on focus/resume - #2623
fix: don't re-request contacts permission on focus/resume#2623kaladivo wants to merge 18 commits into
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults 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:
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Greptile SummaryThe PR separates automatic and user-initiated contact imports so focus/resume refreshes remain passive while the explicit import button may request permission.
Confidence Score: 5/5The PR appears safe to merge, with automatic refreshes now avoiding permission prompts while explicit imports retain their intended behavior. The passive path checks existing permission before entering the contact loader, the explicit button retains the request-capable path, and the existing loader deduplicates overlapping focus and foreground refreshes. Important Files Changed
Sequence DiagramsequenceDiagram
participant L as Focus / app resume
participant B as Import button
participant I as Contact import action
participant P as Permission API
participant C as Contact loader
L->>I: requestPermissions: false
I->>P: Check existing permission
alt Permission already granted
I->>C: Load and normalize contacts
else Permission not granted
I-->>L: Skip import without prompting
end
B->>I: requestPermissions: true
I->>C: Load and normalize contacts
C->>P: Resolve/request permission if needed
Reviews (1): Last reviewed commit: "fix: do not re-request contacts permissi..." | Re-trigger Greptile |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e6fe94f6de
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const permissionsAlreadyGranted = yield* _( | ||
| areContactsPermissionsAlreadyGranted() | ||
| ) | ||
| if (!permissionsAlreadyGranted) return false |
There was a problem hiding this comment.
When permission is denied but canAskAgain remains true and the user already has manual or VCF-imported contacts, this return leaves no way to request access again: ContactsListEmpty exposes the requestPermissions: true action only when normalizedContacts is empty (ContactListSelect/index.tsx:105-132), while ContactsAccessPrivilegesInfoBanner appears only when canAskAgain is false. In that state every focus silently returns here, so importing phone contacts requires finding the OS settings outside this flow; expose an explicit user-initiated import action for the nonempty-list state as well.
Useful? React with 👍 / 👎.
📱 Preview on the staging appOpen Vexl (stage) → Account → Scan QR code and scan this: Preview link:
The preview only loads into staging builds with a matching runtime — if this PR changes native code, ship a new staging build first. To go back to the staging channel: debug screen → "Clear PR preview". |
2 participants
Found while reviewing #2608.
Bug
useContactListSelectLifecyclewas changed to runimportContactsFromPhoneActionAtomfromuseOnFocusAndAppState— i.e. on every screen focus and every app foregrounding (the app-state branch fires even when the screen isn't focused).That action chains into
loadAndNormalizeContactsFromDeviceActionAtom→loadContactsFromDeviceActionAtom→getContactsAndTryToResolveThePermissionsAlongTheWay()→areContactsPermissionsGranted(), which callsrequestPermissionsAsync()whenever permission is not granted butcanAskAgainis true.User-visible consequence: a user who denied the contacts permission once gets the OS contacts permission prompt re-shown on every focus/resume of the Contact preferences screen. Additionally every focus/resume triggered an unthrottled full
getContactsAsync()address-book read.Before #2608 the focus hook only ran the passive
checkContactsAccessPrivilegesActionAtom(getPermissionsAsync, never prompts), andimportContactsFromPhoneActionAtomhad exactly one caller: the explicit "import contacts" button.Fix
importContactsFromPhoneActionAtomnow takes an explicit{requestPermissions: boolean}param:requestPermissions: false— it first checks the passiveareContactsPermissionsAlreadyGranted()and skips loading entirely when access isn't already granted.checkContactsAccessPrivilegesActionAtomstill runs via the existingEffect.ensuring, so the UI keeps reflecting the access state exactly as before feat: Features and fixes #2608.requestPermissions: true, keeping its current prompting behavior.The flag is required (no default) so any future caller has to state its intent instead of silently inheriting the prompting path. This matches how the other two automatic callers guard themselves —
loadContactsFromDeviceActionAtomInAppLoadingTaskandrefreshDeviceContactsSnapshotActionAtomboth gate onareContactsPermissionsAlreadyGranted().Verification
pnpm turbo typecheck,formatandlintall pass for@vexl-next/mobile-app(only pre-existing unrelated deprecation warnings in lint).🤖 Generated with Claude Code