forked from akai/readest
fix(dictionary): correct System Dictionary platform gating on web and iPad (#4362)
* fix(dictionary): keep other dictionaries usable when System Dictionary syncs to an unsupported platform `dictionarySettings.providerEnabled` is whole-field synced across devices, so enabling System Dictionary on macOS/iOS sets the flag on web/Linux/Windows too. There the row is hidden and the feature is a no-op, but the settings UI read the raw flag and locked every other dictionary's toggle read-only. Gate the lock on `isSystemDictionaryEnabled(settings)` — the same platform-aware check the annotator uses — so it matches real lookup behavior and never triggers where the system dictionary can't run. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(dictionary): dispatch system dictionary handoff by native OS (fixes iPad) iPadOS sends a desktop "Macintosh" user agent, so the UA-based `getOSPlatform()` reported iPad as 'macos' and the handoff invoked the macOS-only `show_lookup_popover` Rust command that iOS never registers ("Command show_lookup_popover not found"). Derive the OS from the app service's `is*App` capability flags (sourced from the Tauri OS plugin, correct on iPad) via a new synchronous `getInitializedAppService()` accessor, so iPad routes to the iOS plugin command path. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(ui): constrain reader View Options dropdown to h-8 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * docs(agent): note System Dictionary platform-detection and synced-flag patterns 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:
@@ -114,6 +114,13 @@
|
||||
|
||||
**Fix Strategy:** Scope event handlers to the loaded section's index. Use unique IDs for SVG elements across overlayer instances. Minimize iframe DOM mutations during drag operations.
|
||||
|
||||
### 13. Whole-Field-Synced Flag Reaches an Unsupported Platform
|
||||
**Pattern:** A setting is whole-field synced across devices (e.g. `dictionarySettings.providerEnabled`), so a flag enabled on one platform arrives `true` on a platform where that feature isn't supported. The lookup/runtime path correctly gates on platform support, but a *secondary consumer* (usually UI gating) reads the raw synced flag and misbehaves.
|
||||
**Example:**
|
||||
- System Dictionary enabled on macOS synced to web → web's `CustomDictionaries.tsx` locked all other dictionary toggles read-only (`lockedBySystem`) even though System Dictionary is hidden + a no-op there. The annotator's lookup path used the platform-gated `isSystemDictionaryEnabled(settings)` (registry.ts, gates on `isSystemDictionarySupported()`), but the settings UI compared the raw `providerEnabled[systemDictionary] === true`.
|
||||
|
||||
**Fix Strategy:** Every consumer of a synced flag for a platform-specific feature must route through the *same* platform-aware gate the runtime uses — not the raw `providerEnabled[...]`/setting value. Here: `lockedBySystem = isSystemDictionaryEnabled(settings) && ...`. Search for other readers of the raw flag when fixing one.
|
||||
|
||||
## Debugging Workflow
|
||||
|
||||
1. **Identify the category** from the issue description
|
||||
|
||||
@@ -23,6 +23,12 @@
|
||||
- **CompressionStream** (#3255): Also broken on iOS 15.x; zip.js has its own native API disable
|
||||
- **zip.js native API** (#3170): Disable native `CompressionStream`/`DecompressionStream` on iOS 15.x
|
||||
|
||||
### iPad reports a desktop UA → never branch native dispatch on `getOSPlatform()`
|
||||
- `getOSPlatform()` (utils/misc) is **user-agent based**, and iPadOS sends a desktop "Macintosh" UA → it returns `'macos'` on iPad. Any native-OS dispatch keyed on it misroutes iPad to the macOS path.
|
||||
- Symptom seen: system dictionary on iPad threw `"Command show_lookup_popover not found"` — the macOS-only Rust command (`src-tauri/src/macos/system_dictionary.rs`); iOS only registers the plugin command `plugin:native-bridge|show_lookup_popover`.
|
||||
- **Rule:** for OS-specific native dispatch/capability, use `appService.isIOSApp / isMacOSApp / isAndroidApp` (derived from the Tauri OS plugin `type()` → `OS_TYPE` in `nativeAppService.ts`), NOT `getOSPlatform()`. The misc.ts comment says this explicitly.
|
||||
- Sync, non-React modules: `getInitializedAppService()` (environment.ts) returns the cached singleton synchronously (null pre-init). Used by `systemDictionary.ts` for `isSystemDictionarySupported()` (sync) + `invokeSystemDictionary()` (async).
|
||||
|
||||
### iOS-Specific Code
|
||||
- `src-tauri/plugins/tauri-plugin-native-bridge/ios/Sources/NativeBridgePlugin.swift`
|
||||
- Slider CSS: `-webkit-appearance: none; appearance: none` in globals.css
|
||||
|
||||
@@ -0,0 +1,119 @@
|
||||
/**
|
||||
* CustomDictionaries — system-dictionary exclusivity lock.
|
||||
*
|
||||
* `settings.providerEnabled` is whole-field synced across devices, so the
|
||||
* System Dictionary "enabled" flag can arrive (true) on a device that doesn't
|
||||
* support the OS handoff at all (web, Linux, Windows). On those platforms the
|
||||
* System Dictionary row is hidden and the feature is a no-op at lookup time —
|
||||
* so it must NOT lock the other providers' toggles. On platforms where the
|
||||
* handoff is supported, enabling it stays exclusive and locks the rest.
|
||||
*/
|
||||
import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest';
|
||||
import { render, cleanup } from '@testing-library/react';
|
||||
|
||||
import CustomDictionaries from '@/components/settings/CustomDictionaries';
|
||||
import { useCustomDictionaryStore } from '@/store/customDictionaryStore';
|
||||
import { BUILTIN_PROVIDER_IDS } from '@/services/dictionaries/types';
|
||||
import type { DictionarySettings } from '@/services/dictionaries/types';
|
||||
|
||||
// Per-test platform control. `isSystemDictionaryEnabled` (real, from the
|
||||
// registry) reads `isSystemDictionarySupported`, so toggling these flips both
|
||||
// the row visibility and the lock gate the component now relies on.
|
||||
const platform = vi.hoisted(() => ({ supported: false, available: false }));
|
||||
vi.mock('@/services/dictionaries/systemDictionary', () => ({
|
||||
isSystemDictionarySupported: () => platform.supported,
|
||||
isSystemDictionaryAvailable: () => platform.available,
|
||||
}));
|
||||
|
||||
vi.mock('@/hooks/useTranslation', () => ({
|
||||
useTranslation: () => (s: string) => s,
|
||||
}));
|
||||
|
||||
vi.mock('@/context/EnvContext', () => ({
|
||||
useEnv: () => ({ appService: {}, envConfig: {} }),
|
||||
}));
|
||||
|
||||
vi.mock('@/hooks/useFileSelector', () => ({
|
||||
useFileSelector: () => ({ selectFiles: vi.fn() }),
|
||||
}));
|
||||
|
||||
vi.mock('@/services/sync/replicaBinaryUpload', () => ({
|
||||
queueDictionaryBinaryUpload: vi.fn(),
|
||||
}));
|
||||
|
||||
const LOCKED_TITLE = 'Disable System Dictionary first to change this.';
|
||||
|
||||
const seedSettings = (settings: DictionarySettings) => {
|
||||
useCustomDictionaryStore.setState({
|
||||
dictionaries: [],
|
||||
settings,
|
||||
// The mount effect calls loadCustomDictionaries; no-op it so it can't
|
||||
// clobber the seeded state with on-disk defaults.
|
||||
loadCustomDictionaries: async () => {},
|
||||
saveCustomDictionaries: async () => {},
|
||||
});
|
||||
};
|
||||
|
||||
const enabledSystemSettings: DictionarySettings = {
|
||||
providerOrder: [
|
||||
BUILTIN_PROVIDER_IDS.systemDictionary,
|
||||
BUILTIN_PROVIDER_IDS.wiktionary,
|
||||
BUILTIN_PROVIDER_IDS.wikipedia,
|
||||
],
|
||||
providerEnabled: {
|
||||
// Synced "on" from a device where the OS handoff exists.
|
||||
[BUILTIN_PROVIDER_IDS.systemDictionary]: true,
|
||||
[BUILTIN_PROVIDER_IDS.wiktionary]: true,
|
||||
[BUILTIN_PROVIDER_IDS.wikipedia]: true,
|
||||
},
|
||||
webSearches: [],
|
||||
};
|
||||
|
||||
const getToggles = (container: HTMLElement) =>
|
||||
Array.from(container.querySelectorAll<HTMLInputElement>('input[type="checkbox"]'));
|
||||
|
||||
beforeEach(() => {
|
||||
platform.supported = false;
|
||||
platform.available = false;
|
||||
});
|
||||
|
||||
afterEach(() => {
|
||||
cleanup();
|
||||
});
|
||||
|
||||
describe('CustomDictionaries — system-dictionary lock', () => {
|
||||
it('does not lock other toggles when System Dictionary is unsupported on this platform', () => {
|
||||
// Web: not supported. System Dictionary row is hidden and the synced flag
|
||||
// must not lock Wiktionary / Wikipedia.
|
||||
platform.supported = false;
|
||||
platform.available = false;
|
||||
seedSettings(enabledSystemSettings);
|
||||
|
||||
const { container } = render(<CustomDictionaries onBack={() => {}} />);
|
||||
const toggles = getToggles(container);
|
||||
|
||||
// Two visible rows (System Dictionary hidden on this platform).
|
||||
expect(toggles).toHaveLength(2);
|
||||
expect(toggles.every((t) => !t.disabled)).toBe(true);
|
||||
expect(toggles.some((t) => t.title === LOCKED_TITLE)).toBe(false);
|
||||
});
|
||||
|
||||
it('locks other toggles when System Dictionary is supported and enabled', () => {
|
||||
// macOS: supported. Enabling System Dictionary is exclusive, so the other
|
||||
// providers stay read-only while the System row itself remains toggleable.
|
||||
platform.supported = true;
|
||||
platform.available = true;
|
||||
seedSettings(enabledSystemSettings);
|
||||
|
||||
const { container } = render(<CustomDictionaries onBack={() => {}} />);
|
||||
const toggles = getToggles(container);
|
||||
|
||||
// All three rows visible (System Dictionary first per providerOrder).
|
||||
expect(toggles).toHaveLength(3);
|
||||
const [systemToggle, ...otherToggles] = toggles;
|
||||
expect(systemToggle!.disabled).toBe(false);
|
||||
expect(systemToggle!.title).not.toBe(LOCKED_TITLE);
|
||||
expect(otherToggles.every((t) => t.disabled)).toBe(true);
|
||||
expect(otherToggles.every((t) => t.title === LOCKED_TITLE)).toBe(true);
|
||||
});
|
||||
});
|
||||
@@ -0,0 +1,119 @@
|
||||
/**
|
||||
* System-dictionary native dispatch.
|
||||
*
|
||||
* The handoff must route by the *real* native OS, taken from the app service's
|
||||
* `is*App` capability flags — not the user agent. iPadOS sends a desktop
|
||||
* "Macintosh" UA, so a UA-based check reports iPad as 'macos' and the old
|
||||
* dispatch hit the macOS-only bare `show_lookup_popover` command, which iOS
|
||||
* doesn't register ("Command show_lookup_popover not found"). `appService`
|
||||
* derives its flags from the Tauri OS plugin, so `isIOSApp` is true on iPad
|
||||
* and routes it to the iOS plugin command path.
|
||||
*/
|
||||
import { describe, it, expect, vi, beforeEach } from 'vitest';
|
||||
|
||||
type OsFlags = { isMacOSApp: boolean; isIOSApp: boolean; isAndroidApp: boolean };
|
||||
|
||||
const env = vi.hoisted(() => ({ tauri: true }));
|
||||
const appService = vi.hoisted(
|
||||
() => ({ value: null as OsFlags | null }) as { value: OsFlags | null },
|
||||
);
|
||||
const invokeMock = vi.hoisted(() => vi.fn());
|
||||
|
||||
vi.mock('@/services/environment', () => ({
|
||||
isTauriAppPlatform: () => env.tauri,
|
||||
getInitializedAppService: () => appService.value,
|
||||
}));
|
||||
|
||||
vi.mock('@tauri-apps/api/core', () => ({
|
||||
invoke: (...args: unknown[]) => invokeMock(...args),
|
||||
}));
|
||||
|
||||
vi.mock('@tauri-apps/api/window', () => ({
|
||||
getCurrentWindow: () => ({ label: 'main' }),
|
||||
}));
|
||||
|
||||
import {
|
||||
invokeSystemDictionary,
|
||||
isSystemDictionarySupported,
|
||||
} from '@/services/dictionaries/systemDictionary';
|
||||
|
||||
const MACOS_CMD = 'show_lookup_popover';
|
||||
const PLUGIN_CMD = 'plugin:native-bridge|show_lookup_popover';
|
||||
|
||||
const flags = (os: 'macos' | 'ios' | 'android'): OsFlags => ({
|
||||
isMacOSApp: os === 'macos',
|
||||
isIOSApp: os === 'ios',
|
||||
isAndroidApp: os === 'android',
|
||||
});
|
||||
|
||||
beforeEach(() => {
|
||||
env.tauri = true;
|
||||
// Default to iPad: native ios despite the desktop "Macintosh" UA.
|
||||
appService.value = flags('ios');
|
||||
invokeMock.mockReset();
|
||||
invokeMock.mockImplementation(async (cmd: string) => {
|
||||
if (cmd === PLUGIN_CMD) return { success: true };
|
||||
return undefined; // macOS bare command resolves (no throw) when it exists
|
||||
});
|
||||
});
|
||||
|
||||
describe('invokeSystemDictionary — native dispatch', () => {
|
||||
it('routes iPad (isIOSApp true) to the iOS plugin command', async () => {
|
||||
appService.value = flags('ios'); // iPad: isIOSApp despite the desktop UA
|
||||
|
||||
const ok = await invokeSystemDictionary('hello');
|
||||
|
||||
expect(ok).toBe(true);
|
||||
expect(invokeMock).toHaveBeenCalledWith(PLUGIN_CMD, { payload: { word: 'hello' } });
|
||||
// Must NOT hit the macOS-only Rust command that iOS doesn't register.
|
||||
expect(invokeMock).not.toHaveBeenCalledWith(MACOS_CMD, expect.anything());
|
||||
});
|
||||
|
||||
it('routes a real macOS desktop to the bare Rust command', async () => {
|
||||
appService.value = flags('macos');
|
||||
|
||||
const ok = await invokeSystemDictionary('hello');
|
||||
|
||||
expect(ok).toBe(true);
|
||||
expect(invokeMock).toHaveBeenCalledWith(
|
||||
MACOS_CMD,
|
||||
expect.objectContaining({ word: 'hello', windowLabel: 'main' }),
|
||||
);
|
||||
expect(invokeMock).not.toHaveBeenCalledWith(PLUGIN_CMD, expect.anything());
|
||||
});
|
||||
|
||||
it('routes Android to the plugin command', async () => {
|
||||
appService.value = flags('android');
|
||||
|
||||
const ok = await invokeSystemDictionary('hello');
|
||||
|
||||
expect(ok).toBe(true);
|
||||
expect(invokeMock).toHaveBeenCalledWith(PLUGIN_CMD, { payload: { word: 'hello' } });
|
||||
});
|
||||
|
||||
it('is a no-op when the app service is not yet initialized', async () => {
|
||||
appService.value = null;
|
||||
|
||||
const ok = await invokeSystemDictionary('hello');
|
||||
|
||||
expect(ok).toBe(false);
|
||||
expect(invokeMock).not.toHaveBeenCalled();
|
||||
});
|
||||
});
|
||||
|
||||
describe('isSystemDictionarySupported — appService capability', () => {
|
||||
it('is supported on iPad (isIOSApp true) despite the desktop UA', () => {
|
||||
appService.value = flags('ios');
|
||||
expect(isSystemDictionarySupported()).toBe(true);
|
||||
});
|
||||
|
||||
it('is not supported on web (all is*App flags false)', () => {
|
||||
appService.value = { isMacOSApp: false, isIOSApp: false, isAndroidApp: false };
|
||||
expect(isSystemDictionarySupported()).toBe(false);
|
||||
});
|
||||
|
||||
it('is not supported before the app service is initialized', () => {
|
||||
appService.value = null;
|
||||
expect(isSystemDictionarySupported()).toBe(false);
|
||||
});
|
||||
});
|
||||
@@ -285,6 +285,7 @@ const HeaderBar: React.FC<HeaderBarProps> = ({
|
||||
<NotebookToggler bookKey={bookKey} />
|
||||
<Dropdown
|
||||
label={_('View Options')}
|
||||
containerClassName='h-8'
|
||||
className='exclude-title-bar-mousedown dropdown-bottom dropdown-end'
|
||||
buttonClassName='btn btn-ghost h-8 min-h-8 w-8 p-0 mt-0'
|
||||
toggleButton={<MdOutlineMenu />}
|
||||
|
||||
@@ -28,7 +28,7 @@ import { useTranslation } from '@/hooks/useTranslation';
|
||||
import { useFileSelector } from '@/hooks/useFileSelector';
|
||||
import { useCustomDictionaryStore } from '@/store/customDictionaryStore';
|
||||
import { eventDispatcher } from '@/utils/event';
|
||||
import { evictProvider } from '@/services/dictionaries/registry';
|
||||
import { evictProvider, isSystemDictionaryEnabled } from '@/services/dictionaries/registry';
|
||||
import { BUILTIN_PROVIDER_IDS } from '@/services/dictionaries/types';
|
||||
import {
|
||||
isSystemDictionaryAvailable,
|
||||
@@ -448,6 +448,14 @@ const CustomDictionaries: React.FC<CustomDictionariesProps> = ({ onBack }) => {
|
||||
const rows = buildRows();
|
||||
const hasDeletable = rows.some((r) => r.imported || (r.kind === 'web' && !r.builtinWeb));
|
||||
|
||||
// System-dictionary handoff is exclusive at lookup time — but only on
|
||||
// platforms where it's actually supported. `providerEnabled` is whole-field
|
||||
// synced across devices, so the flag can arrive (true) on web / Linux /
|
||||
// Windows where there's no handoff; there it's a no-op and must NOT lock the
|
||||
// other providers' toggles. `isSystemDictionaryEnabled` applies the same
|
||||
// platform gate the annotator uses, so the lock matches real lookup behavior.
|
||||
const systemDictionaryActive = isSystemDictionaryEnabled(settings);
|
||||
|
||||
// dnd-kit sensors. PointerSensor with a small distance gate avoids
|
||||
// hijacking simple clicks on the drag handle. TouchSensor with a delay
|
||||
// matches mobile UX (long-press to drag). Keyboard support gives drag
|
||||
@@ -718,8 +726,7 @@ const CustomDictionaries: React.FC<CustomDictionariesProps> = ({ onBack }) => {
|
||||
// read-only so the user can't accidentally clear that
|
||||
// restoration state.
|
||||
lockedBySystem={
|
||||
settings.providerEnabled[BUILTIN_PROVIDER_IDS.systemDictionary] === true &&
|
||||
row.id !== BUILTIN_PROVIDER_IDS.systemDictionary
|
||||
systemDictionaryActive && row.id !== BUILTIN_PROVIDER_IDS.systemDictionary
|
||||
}
|
||||
isDeleteMode={isDeleteMode}
|
||||
isEditMode={isEditMode}
|
||||
|
||||
@@ -30,10 +30,31 @@
|
||||
*/
|
||||
import { invoke } from '@tauri-apps/api/core';
|
||||
import { getCurrentWindow } from '@tauri-apps/api/window';
|
||||
import { isTauriAppPlatform } from '@/services/environment';
|
||||
import { getOSPlatform } from '@/utils/misc';
|
||||
import { getInitializedAppService, isTauriAppPlatform } from '@/services/environment';
|
||||
import type { Rect } from '@/utils/sel';
|
||||
|
||||
/**
|
||||
* Real native OS for the handoff, taken from the app service's capability
|
||||
* flags rather than the user agent.
|
||||
*
|
||||
* `getOSPlatform()` (utils/misc) is UA-based, and iPadOS sends a desktop
|
||||
* "Macintosh" user agent — so it reports iPad as 'macos'. Dispatching on that
|
||||
* would route the handoff to the macOS-only `show_lookup_popover` Rust command
|
||||
* that iOS never registers, yielding "Command show_lookup_popover not found"
|
||||
* on iPad. The app service's `is*App` flags derive from the Tauri OS plugin
|
||||
* (see `nativeAppService`), so `isIOSApp` is correctly true on iPad. Returns
|
||||
* 'unknown' before the service is initialized or on web (where the flags are
|
||||
* all false), which the callers treat as "unsupported".
|
||||
*/
|
||||
const getSystemDictionaryOS = (): 'macos' | 'ios' | 'android' | 'unknown' => {
|
||||
const appService = getInitializedAppService();
|
||||
if (!appService) return 'unknown';
|
||||
if (appService.isMacOSApp) return 'macos';
|
||||
if (appService.isIOSApp) return 'ios';
|
||||
if (appService.isAndroidApp) return 'android';
|
||||
return 'unknown';
|
||||
};
|
||||
|
||||
/**
|
||||
* Optional positional hint for the lookup HUD (macOS only). When
|
||||
* provided, the macOS bridge anchors the popover near the selection's
|
||||
@@ -72,8 +93,7 @@ export interface SystemDictionaryAnchorStyle {
|
||||
* popup tab list on unsupported hosts.
|
||||
*/
|
||||
export const isSystemDictionarySupported = (): boolean => {
|
||||
if (!isTauriAppPlatform()) return false;
|
||||
const os = typeof navigator !== 'undefined' ? getOSPlatform() : 'unknown';
|
||||
const os = getSystemDictionaryOS();
|
||||
return os === 'macos' || os === 'ios' || os === 'android';
|
||||
};
|
||||
|
||||
@@ -115,7 +135,7 @@ export const invokeSystemDictionary = async (
|
||||
if (!trimmed) return false;
|
||||
if (!isTauriAppPlatform()) return false;
|
||||
|
||||
const os = getOSPlatform();
|
||||
const os = getSystemDictionaryOS();
|
||||
try {
|
||||
if (os === 'macos') {
|
||||
// Calls the Rust `show_lookup_popover` command in
|
||||
|
||||
@@ -69,4 +69,13 @@ const environmentConfig: EnvConfigType = {
|
||||
},
|
||||
};
|
||||
|
||||
/**
|
||||
* Synchronously returns the app service if it has already been created by
|
||||
* {@link environmentConfig.getAppService}; null before first init. The async
|
||||
* getter is preferred everywhere — use this only from synchronous code paths
|
||||
* that run well after startup (e.g. capability checks during reader render),
|
||||
* where the singleton is guaranteed to exist.
|
||||
*/
|
||||
export const getInitializedAppService = (): AppService | null => nativeAppService ?? webAppService;
|
||||
|
||||
export default environmentConfig;
|
||||
|
||||
Reference in New Issue
Block a user