forked from akai/readest
fix: real fix for library-save storage-permission crash + narrowed view-transition filter (#4943)
* fix(library): request storage permission when saving to a custom folder On Android a custom library folder on shared storage needs All Files Access. The import/settings/migrate paths request it, but the library-save path (updateBook/updateBooks -> saveLibraryBooks) did not, so on a device without the permission every save failed with EACCES; because callers (sync, imports) don't await/catch it, it surfaced as an unhandled-rejection crash (Sentry READEST-A). saveLibraryBooks now catches a storage-permission error, requests the permission through the existing flow, and retries once. It prompts at most once per session so background saves don't repeatedly open system settings; a still-denied save is logged rather than crashing (the user was already shown the All Files Access screen). Fixes READEST-A Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(sentry): drop only the benign hidden-tab View Transition error The View Transition API skips a transition when the tab is hidden; that unhandled rejection is expected browser behavior and pure noise, so it is dropped in before_send. A transition timeout abort (READEST-9) is NOT dropped: a slow DOM update can signal a real performance problem, so it stays visible. Fixes READEST-7 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This commit is contained in:
@@ -321,6 +321,15 @@ pub fn run() {
|
||||
// the OS as "Linux"; relabel it "Android" (and recover the Android
|
||||
// version from the kernel string) so events group correctly.
|
||||
before_send: Some(std::sync::Arc::new(|mut event| {
|
||||
// Drop known-benign browser noise (e.g. View Transition
|
||||
// skipped/aborted) before it is reported.
|
||||
if event.exception.values.iter().any(|ex| {
|
||||
ex.value
|
||||
.as_deref()
|
||||
.is_some_and(sentry_config::is_ignored_browser_error)
|
||||
}) {
|
||||
return None;
|
||||
}
|
||||
if let Some(sentry::protocol::Context::Os(os)) = event.contexts.get_mut("os") {
|
||||
if let Some(name) = sentry_config::corrected_os_name(
|
||||
std::env::consts::OS,
|
||||
|
||||
@@ -76,6 +76,15 @@ pub fn android_version_from_uname(release: &str) -> Option<String> {
|
||||
}
|
||||
}
|
||||
|
||||
/// A known-benign browser error that is expected behavior, not an app bug, and
|
||||
/// only adds noise to crash reporting: the View Transition API skips a
|
||||
/// transition when the tab is hidden. Matched on the exception value so it is
|
||||
/// dropped in `before_send`. NOTE: a transition *timeout* abort is deliberately
|
||||
/// NOT ignored — a slow DOM update can signal a real performance problem.
|
||||
pub fn is_ignored_browser_error(value: &str) -> bool {
|
||||
value.contains("View transition was skipped because document visibility state is hidden")
|
||||
}
|
||||
|
||||
/// C-ABI accessor for the compile-time Sentry DSN, used by the iOS native
|
||||
/// bootstrap (sentry-cocoa) so it starts with the same DSN as the Rust client
|
||||
/// without a second env read or fragile Info.plist / preprocessor plumbing.
|
||||
@@ -97,7 +106,7 @@ pub extern "C" fn readest_sentry_dsn() -> *const std::os::raw::c_char {
|
||||
mod tests {
|
||||
use super::{
|
||||
android_version_from_uname, corrected_os_name, dsn_from_env, environment_for_version,
|
||||
release_name,
|
||||
is_ignored_browser_error, release_name,
|
||||
};
|
||||
|
||||
#[test]
|
||||
@@ -165,4 +174,22 @@ mod tests {
|
||||
assert_eq!(android_version_from_uname(""), None);
|
||||
assert_eq!(android_version_from_uname("6.1.162-androidX"), None);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn ignores_benign_hidden_view_transition_error() {
|
||||
assert!(is_ignored_browser_error(
|
||||
"InvalidStateError: View transition was skipped because document visibility state is hidden."
|
||||
));
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn keeps_view_transition_timeout_and_real_errors() {
|
||||
// A transition timeout can signal a real perf problem — do NOT suppress it.
|
||||
assert!(!is_ignored_browser_error(
|
||||
"TimeoutError: Transition was aborted because of timeout in DOM update"
|
||||
));
|
||||
assert!(!is_ignored_browser_error("TypeError: Load failed"));
|
||||
assert!(!is_ignored_browser_error("concurrent use forbidden"));
|
||||
assert!(!is_ignored_browser_error(""));
|
||||
}
|
||||
}
|
||||
|
||||
@@ -67,9 +67,20 @@ vi.mock('@/utils/misc', async (importOriginal) => {
|
||||
};
|
||||
});
|
||||
|
||||
// Keep the real isStoragePermissionError; only stub the interactive request.
|
||||
vi.mock('@/utils/permission', async (importOriginal) => {
|
||||
const original = await importOriginal<Record<string, unknown>>();
|
||||
return {
|
||||
...original,
|
||||
requestStoragePermission: vi.fn(),
|
||||
};
|
||||
});
|
||||
|
||||
import { BaseAppService } from '@/services/appService';
|
||||
import * as Settings from '@/services/settingsService';
|
||||
import * as BookSvc from '@/services/bookService';
|
||||
import * as LibrarySvc from '@/services/libraryService';
|
||||
import { requestStoragePermission } from '@/utils/permission';
|
||||
|
||||
// Concrete test implementation of BaseAppService
|
||||
class TestAppService extends BaseAppService {
|
||||
@@ -193,6 +204,70 @@ describe('BaseAppService', () => {
|
||||
});
|
||||
});
|
||||
|
||||
describe('saveLibraryBooks storage permission (READEST-A)', () => {
|
||||
// clearAllMocks resets call history but not implementations, so restore the
|
||||
// shared saveLibraryBooks mock's default after these override it.
|
||||
afterEach(() => {
|
||||
vi.mocked(LibrarySvc.saveLibraryBooks).mockReset().mockResolvedValue(undefined);
|
||||
vi.mocked(requestStoragePermission).mockReset();
|
||||
});
|
||||
|
||||
const permError = () =>
|
||||
new Error(
|
||||
'Failed to save library.json: failed to open file at path: ' +
|
||||
'/storage/emulated/0/Readest/Books/library.json.bak with error: ' +
|
||||
'Permission denied (os error 13)',
|
||||
);
|
||||
|
||||
test('on Android, requests storage permission and retries once when granted', async () => {
|
||||
service.isAndroidApp = true;
|
||||
vi.mocked(LibrarySvc.saveLibraryBooks)
|
||||
.mockRejectedValueOnce(permError())
|
||||
.mockResolvedValueOnce(undefined);
|
||||
vi.mocked(requestStoragePermission).mockResolvedValue(true);
|
||||
|
||||
await expect(service.saveLibraryBooks([])).resolves.toBeUndefined();
|
||||
expect(requestStoragePermission).toHaveBeenCalledTimes(1);
|
||||
expect(LibrarySvc.saveLibraryBooks).toHaveBeenCalledTimes(2);
|
||||
});
|
||||
|
||||
test('on Android, does not crash when permission is denied', async () => {
|
||||
service.isAndroidApp = true;
|
||||
vi.mocked(LibrarySvc.saveLibraryBooks).mockRejectedValue(permError());
|
||||
vi.mocked(requestStoragePermission).mockResolvedValue(false);
|
||||
|
||||
await expect(service.saveLibraryBooks([])).resolves.toBeUndefined();
|
||||
// No retry when the permission was declined.
|
||||
expect(LibrarySvc.saveLibraryBooks).toHaveBeenCalledTimes(1);
|
||||
});
|
||||
|
||||
test('only prompts once per session across repeated failing saves', async () => {
|
||||
service.isAndroidApp = true;
|
||||
vi.mocked(LibrarySvc.saveLibraryBooks).mockRejectedValue(permError());
|
||||
vi.mocked(requestStoragePermission).mockResolvedValue(false);
|
||||
|
||||
await service.saveLibraryBooks([]);
|
||||
await service.saveLibraryBooks([]);
|
||||
expect(requestStoragePermission).toHaveBeenCalledTimes(1);
|
||||
});
|
||||
|
||||
test('re-throws non-permission errors', async () => {
|
||||
service.isAndroidApp = true;
|
||||
vi.mocked(LibrarySvc.saveLibraryBooks).mockRejectedValue(new Error('disk full'));
|
||||
|
||||
await expect(service.saveLibraryBooks([])).rejects.toThrow('disk full');
|
||||
expect(requestStoragePermission).not.toHaveBeenCalled();
|
||||
});
|
||||
|
||||
test('does not intercept on non-Android platforms', async () => {
|
||||
service.isAndroidApp = false;
|
||||
vi.mocked(LibrarySvc.saveLibraryBooks).mockRejectedValue(permError());
|
||||
|
||||
await expect(service.saveLibraryBooks([])).rejects.toThrow('Permission denied');
|
||||
expect(requestStoragePermission).not.toHaveBeenCalled();
|
||||
});
|
||||
});
|
||||
|
||||
describe('file operations', () => {
|
||||
test('openFile delegates to fs', async () => {
|
||||
await service.openFile('test.epub', 'Books');
|
||||
|
||||
@@ -0,0 +1,23 @@
|
||||
import { describe, test, expect } from 'vitest';
|
||||
import { isStoragePermissionError } from '@/utils/permission';
|
||||
|
||||
describe('isStoragePermissionError', () => {
|
||||
test('detects Android EACCES / permission-denied save failures', () => {
|
||||
expect(
|
||||
isStoragePermissionError(
|
||||
new Error(
|
||||
'Failed to save library.json: failed to open file at path: ' +
|
||||
'/storage/emulated/0/Readest/Books/library.json.bak with error: ' +
|
||||
'Permission denied (os error 13)',
|
||||
),
|
||||
),
|
||||
).toBe(true);
|
||||
expect(isStoragePermissionError('EACCES: permission denied')).toBe(true);
|
||||
});
|
||||
|
||||
test('ignores unrelated errors', () => {
|
||||
expect(isStoragePermissionError(new Error('disk full'))).toBe(false);
|
||||
expect(isStoragePermissionError(new Error('JSON parse error'))).toBe(false);
|
||||
expect(isStoragePermissionError(undefined)).toBe(false);
|
||||
});
|
||||
});
|
||||
@@ -19,6 +19,7 @@ import type { BookNav } from '@/services/nav';
|
||||
import { getLibraryFilename, getLibraryBackupFilename } from '@/utils/book';
|
||||
|
||||
import { getOSPlatform } from '@/utils/misc';
|
||||
import { isStoragePermissionError, requestStoragePermission } from '@/utils/permission';
|
||||
import { ProgressHandler } from '@/utils/transfer';
|
||||
import { CustomTextureInfo } from '@/styles/textures';
|
||||
import { CustomFont, CustomFontInfo } from '@/styles/fonts';
|
||||
@@ -441,7 +442,31 @@ export abstract class BaseAppService implements AppService {
|
||||
return LibrarySvc.loadLibraryBooks(this.fs, this.generateCoverImageUrl.bind(this));
|
||||
}
|
||||
|
||||
// Prompt for storage permission at most once per session (see saveLibraryBooks).
|
||||
private storagePermissionRequested = false;
|
||||
|
||||
async saveLibraryBooks(books: Book[], options?: SaveLibraryBooksOptions): Promise<void> {
|
||||
return LibrarySvc.saveLibraryBooks(this.fs, books, options);
|
||||
try {
|
||||
return await LibrarySvc.saveLibraryBooks(this.fs, books, options);
|
||||
} catch (error) {
|
||||
// A custom library folder on Android shared storage needs All Files
|
||||
// Access. Without it the write fails with EACCES and, because callers
|
||||
// (sync, imports) don't await/catch this, it surfaced as an unhandled
|
||||
// rejection crash (Sentry READEST-A). Re-request the permission through
|
||||
// the same flow used at import time and retry once. Only prompt once per
|
||||
// session so background saves don't repeatedly yank the user to system
|
||||
// settings; after that a still-denied save is logged, not crashed —
|
||||
// the user was already shown the All Files Access screen.
|
||||
if (!this.isAndroidApp || !isStoragePermissionError(error)) {
|
||||
throw error;
|
||||
}
|
||||
if (!this.storagePermissionRequested) {
|
||||
this.storagePermissionRequested = true;
|
||||
if (await requestStoragePermission()) {
|
||||
return await LibrarySvc.saveLibraryBooks(this.fs, books, options);
|
||||
}
|
||||
}
|
||||
console.warn('[library] not saved: storage permission not granted', error);
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
@@ -4,6 +4,16 @@ interface Permissions {
|
||||
manageStorage: PermissionState;
|
||||
}
|
||||
|
||||
/**
|
||||
* Whether a thrown error is an Android storage-permission denial (EACCES). A
|
||||
* custom library folder on shared storage needs All Files Access; without it
|
||||
* file writes fail with "Permission denied (os error 13)".
|
||||
*/
|
||||
export const isStoragePermissionError = (error: unknown): boolean => {
|
||||
const message = error instanceof Error ? error.message : String(error);
|
||||
return /os error 13|permission denied|eacces/i.test(message);
|
||||
};
|
||||
|
||||
export const requestStoragePermission = async (): Promise<boolean> => {
|
||||
let permission = await invoke<Permissions>('plugin:native-bridge|checkPermissions');
|
||||
if (permission.manageStorage !== 'granted') {
|
||||
|
||||
Reference in New Issue
Block a user