Skip to content

fix(android): remove unnecessary notification permission request #314

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
dishit-wednesday merged 2 commits into from
Apr 21, 2026
Merged

fix(android): remove unnecessary notification permission request #314

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
13e6369
fix(android): remove unnecessary notification permission request
dishit-wednesday Apr 21, 2026
8bad738
fix: address gemini review — add download handler tests, remove log file
dishit-wednesday Apr 21, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
116 changes: 0 additions & 116 deletions __tests__/unit/hooks/useNotifRationale.test.ts
View file
Open in desktop

This file was deleted.

54 changes: 6 additions & 48 deletions __tests__/unit/screens/ModelsScreen/useModelsScreen.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -141,16 +141,6 @@ jest.mock('../../../../src/screens/ModelsScreen/useImageModels', () => ({
})),
}));

// Mock useNotifRationale
jest.mock('../../../../src/screens/ModelsScreen/useNotifRationale', () => ({
useNotifRationale: jest.fn(() => ({
showNotifRationale: false,
maybeShowNotifRationale: jest.fn((cb) => cb()),
handleNotifRationaleAllow: jest.fn(),
handleNotifRationaleDismiss: jest.fn(),
})),
}));

// Mock useAppStore
jest.mock('../../../../src/stores', () => ({
useAppStore: jest.fn(() => ({
Expand Down Expand Up @@ -563,19 +553,10 @@ describe('useModelsScreen', () => {
});

describe('handleDownload callback', () => {

@gemini-code-assist gemini-code-assist Bot Apr 21, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

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.

it('calls maybeShowNotifRationale with download handler', () => {
const { useNotifRationale } = require('../../../../src/screens/ModelsScreen/useNotifRationale');
const mockMaybeShowNotifRationale = jest.fn();
it('calls text.handleDownload directly with correct args', () => {
const { useTextModels } = require('../../../../src/screens/ModelsScreen/useTextModels');
const mockHandleDownload = jest.fn();

useNotifRationale.mockReturnValue({
showNotifRationale: false,
maybeShowNotifRationale: mockMaybeShowNotifRationale,
handleNotifRationaleAllow: jest.fn(),
handleNotifRationaleDismiss: jest.fn(),
});

const { useTextModels } = require('../../../../src/screens/ModelsScreen/useTextModels');
useTextModels.mockReturnValue({
downloadedModels: [],
setIsRefreshing: jest.fn(),
Expand All @@ -590,36 +571,22 @@ describe('useModelsScreen', () => {
});

const { result } = renderHook(() => useModelsScreen());

const mockModel: any = { id: 'model-id', name: 'Test', author: 'Test', files: [] };
const mockFile: any = { name: 'url', size: 100, quantization: 'Q4', downloadUrl: 'http://test' };

act(() => {
result.current.handleDownload(mockModel, mockFile);
});

expect(mockMaybeShowNotifRationale).toHaveBeenCalled();
// The callback passed to maybeShowNotifRationale should call handleDownload
const callback = mockMaybeShowNotifRationale.mock.calls[0][0];
callback();
expect(mockHandleDownload).toHaveBeenCalledWith(mockModel, mockFile);
});
});

describe('handleDownloadImageModel callback', () => {
it('calls maybeShowNotifRationale with image download handler', () => {
const { useNotifRationale } = require('../../../../src/screens/ModelsScreen/useNotifRationale');
const mockMaybeShowNotifRationale = jest.fn();
it('calls image.handleDownloadImageModel directly with correct args', () => {
const { useImageModels } = require('../../../../src/screens/ModelsScreen/useImageModels');
const mockHandleDownloadImageModel = jest.fn();

useNotifRationale.mockReturnValue({
showNotifRationale: false,
maybeShowNotifRationale: mockMaybeShowNotifRationale,
handleNotifRationaleAllow: jest.fn(),
handleNotifRationaleDismiss: jest.fn(),
});

const { useImageModels } = require('../../../../src/screens/ModelsScreen/useImageModels');
useImageModels.mockReturnValue({
downloadedImageModels: [],
loadDownloadedImageModels: jest.fn().mockResolvedValue(undefined),
Expand All @@ -631,24 +598,15 @@ describe('useModelsScreen', () => {
});

const { result } = renderHook(() => useModelsScreen());

const mockImageModel: any = {
id: 'img-model',
name: 'Test Model',
description: 'Test',
downloadUrl: 'http://test',
size: 100,
style: 'default',
backend: 'mnn'
id: 'img-model', name: 'Test Model', description: 'Test',
downloadUrl: 'http://test', size: 100, style: 'default', backend: 'mnn',
};

act(() => {
result.current.handleDownloadImageModel(mockImageModel);
});

expect(mockMaybeShowNotifRationale).toHaveBeenCalled();
const callback = mockMaybeShowNotifRationale.mock.calls[0][0];
callback();
expect(mockHandleDownloadImageModel).toHaveBeenCalledWith(mockImageModel);
});
});
Expand Down
70 changes: 0 additions & 70 deletions __tests__/unit/services/backgroundDownloadService.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -995,76 +995,6 @@ describe('BackgroundDownloadService', () => {
});
});

// ========================================================================
// requestNotificationPermission
// ========================================================================
describe('requestNotificationPermission', () => {
const { PermissionsAndroid } = require('react-native');

beforeEach(() => {
PermissionsAndroid.request = jest.fn().mockResolvedValue('granted');
});

it('requests POST_NOTIFICATIONS on Android API 33+', async () => {
Object.defineProperty(Platform, 'OS', { get: () => 'android' });
Object.defineProperty(Platform, 'Version', { get: () => 33 });

await service.requestNotificationPermission();

expect(PermissionsAndroid.request).toHaveBeenCalledWith(
PermissionsAndroid.PERMISSIONS.POST_NOTIFICATIONS,
);
});

it('requests on API 34', async () => {
Object.defineProperty(Platform, 'OS', { get: () => 'android' });
Object.defineProperty(Platform, 'Version', { get: () => 34 });

await service.requestNotificationPermission();

expect(PermissionsAndroid.request).toHaveBeenCalled();
});

it('does nothing on iOS', async () => {
Object.defineProperty(Platform, 'OS', { get: () => 'ios' });

await service.requestNotificationPermission();

expect(PermissionsAndroid.request).not.toHaveBeenCalled();
});

it('does nothing on Android API 32', async () => {
Object.defineProperty(Platform, 'OS', { get: () => 'android' });
Object.defineProperty(Platform, 'Version', { get: () => 32 });

await service.requestNotificationPermission();

expect(PermissionsAndroid.request).not.toHaveBeenCalled();
});

it('does not throw when permission request rejects', async () => {
Object.defineProperty(Platform, 'OS', { get: () => 'android' });
Object.defineProperty(Platform, 'Version', { get: () => 33 });
PermissionsAndroid.request = jest
.fn()
.mockRejectedValue(new Error('Permission error'));

await expect(
service.requestNotificationPermission(),
).resolves.toBeUndefined();
});

it('handles denied permission without throwing', async () => {
Object.defineProperty(Platform, 'OS', { get: () => 'android' });
Object.defineProperty(Platform, 'Version', { get: () => 33 });
PermissionsAndroid.request = jest.fn().mockResolvedValue('denied');

await expect(
service.requestNotificationPermission(),
).resolves.toBeUndefined();
});
});

// ========================================================================
// downloadFileTo
// ========================================================================
Expand Down
3 changes: 0 additions & 3 deletions android/app/src/main/AndroidManifest.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,9 +17,6 @@
<uses-permission android:name="android.permission.READ_EXTERNAL_STORAGE"
android:maxSdkVersion="32" />

<!-- For download notifications (Android 13+) -->
<uses-permission android:name="android.permission.POST_NOTIFICATIONS" />

<!-- For silent companion file downloads (e.g. mmproj for vision models) -->
<uses-permission android:name="android.permission.DOWNLOAD_WITHOUT_NOTIFICATION" />

Expand Down
10 changes: 0 additions & 10 deletions src/screens/ModelsScreen/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -213,16 +213,6 @@ export const ModelsScreen: React.FC = () => {
)}

<CustomAlert {...vm.alertState} onClose={() => vm.setAlertState(hideAlert())} />
<CustomAlert
visible={vm.showNotifRationale}
title="Download notifications"
message={"Off Grid can show download progress in your notification tray while you use other apps.\n\nThis is optional — downloads work fine without it."}
onClose={vm.handleNotifRationaleDismiss}
buttons={[
{ text: 'No thanks', style: 'cancel', onPress: vm.handleNotifRationaleDismiss },
{ text: 'Allow', style: 'default', onPress: vm.handleNotifRationaleAllow },
]}
/>
</SafeAreaView>
);
};
Expand Down
21 changes: 4 additions & 17 deletions src/screens/ModelsScreen/useModelsScreen.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,6 @@ import { initialFilterState } from './constants';
import { getDirectorySize } from './utils';
import { useTextModels } from './useTextModels';
import { useImageModels } from './useImageModels';
import { useNotifRationale } from './useNotifRationale';
import { importGgufFiles, getErrorMessage } from './importHelpers';
import { isPickerStuck } from '../../utils/pickerErrorUtils';

Expand Down Expand Up @@ -85,15 +84,6 @@ export function useModelsScreen() {
const text = useTextModels(setAlertState);
const image = useImageModels(setAlertState);

const isFirstDownload =
text.downloadedModels.length === 0 && image.downloadedImageModels.length === 0;
const {
showNotifRationale,
maybeShowNotifRationale,
handleNotifRationaleAllow,
handleNotifRationaleDismiss,
} = useNotifRationale(isFirstDownload);

useEffect(() => {
if (activeTab === 'image' && image.availableHFModels.length === 0 && !image.hfModelsLoading) {
image.loadHFModels();
Expand Down Expand Up @@ -194,16 +184,16 @@ export function useModelsScreen() {

const handleDownload = useCallback(
(...args: Parameters<typeof text.handleDownload>) => {
maybeShowNotifRationale(() => text.handleDownload(...args));
text.handleDownload(...args);

@gemini-code-assist gemini-code-assist Bot Apr 21, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

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).

},
[maybeShowNotifRationale, text],
[text],
);

const handleDownloadImageModel = useCallback(
(...args: Parameters<typeof image.handleDownloadImageModel>) => {
maybeShowNotifRationale(() => image.handleDownloadImageModel(...args));
image.handleDownloadImageModel(...args);
},
[maybeShowNotifRationale, image],
[image],
);

return {
Expand Down Expand Up @@ -290,9 +280,6 @@ export function useModelsScreen() {
isRecommendedModel: image.isRecommendedModel,
handleDownloadImageModel,
handleCancelImageDownload: image.handleCancelImageDownload,
showNotifRationale,
handleNotifRationaleAllow,
handleNotifRationaleDismiss,
setUserChangedBackendFilter: image.setUserChangedBackendFilter,
};
}
Expand Down
Loading
Loading