fix(android): remove unnecessary notification permission request#314
Conversation
POST_NOTIFICATIONS was only needed for the foreground service notification. Now that downloads run via WorkManager silently, there are no system notifications to show. Removes the permission from the manifest, the runtime request logic, and all related UI. Co-authored-by: Dishit <hanmadishit74@gmail.com>
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request removes the notification rationale feature, including the useNotifRationale hook, its corresponding UI components, and the POST_NOTIFICATIONS permission. It also simplifies the download logic in useModelsScreen by removing the rationale check. Feedback indicates that a development log file was accidentally committed and should be removed. Furthermore, the reviewer suggests updating the tests for download handlers rather than deleting them and simplifying the pass-through useCallback wrappers in useModelsScreen.
| @@ -0,0 +1,688 @@ | |||
| 2026-04-21T05:07:25.740Z] LOG: [ProviderRegistry] Registered provider: local | |||
There was a problem hiding this comment.
This log file appears to have been committed accidentally. It contains detailed runtime logs from a development session and should be removed from the repository to avoid unnecessary bloat and potential exposure of local environment details.
| }); | ||
| }); | ||
|
|
||
| describe('handleDownload callback', () => { |
There was a problem hiding this comment.
The tests for handleDownload and handleDownloadImageModel were removed entirely. While the rationale logic is gone, these functions still exist in the hook and should have updated tests to verify they correctly invoke the underlying download handlers from useTextModels and useImageModels respectively.
| const handleDownload = useCallback( | ||
| (...args: Parameters<typeof text.handleDownload>) => { | ||
| maybeShowNotifRationale(() => text.handleDownload(...args)); | ||
| text.handleDownload(...args); |
There was a problem hiding this comment.
This useCallback wrapper is now a simple pass-through to text.handleDownload. To simplify the code and avoid unnecessary re-renders (if the text object is not memoized), consider removing this wrapper and returning text.handleDownload directly from the hook in the return object (line 236).
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #314 +/- ##
==========================================
- Coverage 83.86% 83.83% -0.03%
==========================================
Files 224 223 -1
Lines 11490 11460 -30
Branches 3148 3144 -4
==========================================
- Hits 9636 9608 -28
+ Misses 1071 1070 -1
+ Partials 783 782 -1
🚀 New features to boost your workflow:
|
Replace removed notif-rationale test stubs with updated tests that verify handleDownload and handleDownloadImageModel delegate directly to their underlying handlers. Delete accidentally committed dev log. Co-authored-by: Dishit hanmadishit74@gmail.com
dishit-wednesday
force-pushed
the
fix/remove-notification-permission
branch
from
f601cf2 to
8bad738
Compare
|
1 participant
POST_NOTIFICATIONS was only needed for the foreground service notification. Now that downloads run via WorkManager silently, there are no system notifications to show. Removes the permission from the manifest, the runtime request logic, and all related UI.
Summary
Type of Change
Screenshots / Screen Recordings
Android
iOS
Checklist
General
Testing
npm test)React Native Specific
project.pbxproj)SPACING/TYPOGRAPHYconstants from the themeuseThemedStylespattern (not inline or staticStyleSheet.create)FlatList/FlashList(not.map()insideScrollView)Performance & Models
/vs\\)Security
Related Issues
Additional Notes