From 78794499a26f36410581bea4efa1d261d01ef216 Mon Sep 17 00:00:00 2001 From: Huang Xin Date: Sat, 30 May 2026 15:51:22 +0800 Subject: [PATCH] fix(dictionary): correct System Dictionary platform gating on web and iPad (#4362) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * 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) * 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) * fix(ui): constrain reader View Options dropdown to h-8 Co-Authored-By: Claude Opus 4.8 (1M context) * docs(agent): note System Dictionary platform-detection and synced-flag patterns Co-Authored-By: Claude Opus 4.8 (1M context) --------- Co-authored-by: Claude Opus 4.8 (1M context) --- .../.claude/memory/bug-patterns.md | 7 ++ .../.claude/memory/platform-compat-fixes.md | 6 + .../settings/CustomDictionaries.test.tsx | 119 ++++++++++++++++++ .../dictionaries/systemDictionary.test.ts | 119 ++++++++++++++++++ .../src/app/reader/components/HeaderBar.tsx | 1 + .../settings/CustomDictionaries.tsx | 13 +- .../services/dictionaries/systemDictionary.ts | 30 ++++- apps/readest-app/src/services/environment.ts | 9 ++ 8 files changed, 296 insertions(+), 8 deletions(-) create mode 100644 apps/readest-app/src/__tests__/components/settings/CustomDictionaries.test.tsx create mode 100644 apps/readest-app/src/__tests__/services/dictionaries/systemDictionary.test.ts diff --git a/apps/readest-app/.claude/memory/bug-patterns.md b/apps/readest-app/.claude/memory/bug-patterns.md index 26eb39c8..c850e421 100644 --- a/apps/readest-app/.claude/memory/bug-patterns.md +++ b/apps/readest-app/.claude/memory/bug-patterns.md @@ -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 diff --git a/apps/readest-app/.claude/memory/platform-compat-fixes.md b/apps/readest-app/.claude/memory/platform-compat-fixes.md index feb5531f..6fc53182 100644 --- a/apps/readest-app/.claude/memory/platform-compat-fixes.md +++ b/apps/readest-app/.claude/memory/platform-compat-fixes.md @@ -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 diff --git a/apps/readest-app/src/__tests__/components/settings/CustomDictionaries.test.tsx b/apps/readest-app/src/__tests__/components/settings/CustomDictionaries.test.tsx new file mode 100644 index 00000000..dd9424c5 --- /dev/null +++ b/apps/readest-app/src/__tests__/components/settings/CustomDictionaries.test.tsx @@ -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('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( {}} />); + 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( {}} />); + 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); + }); +}); diff --git a/apps/readest-app/src/__tests__/services/dictionaries/systemDictionary.test.ts b/apps/readest-app/src/__tests__/services/dictionaries/systemDictionary.test.ts new file mode 100644 index 00000000..feb8d8a3 --- /dev/null +++ b/apps/readest-app/src/__tests__/services/dictionaries/systemDictionary.test.ts @@ -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); + }); +}); diff --git a/apps/readest-app/src/app/reader/components/HeaderBar.tsx b/apps/readest-app/src/app/reader/components/HeaderBar.tsx index 93591abe..5b9d57b6 100644 --- a/apps/readest-app/src/app/reader/components/HeaderBar.tsx +++ b/apps/readest-app/src/app/reader/components/HeaderBar.tsx @@ -285,6 +285,7 @@ const HeaderBar: React.FC = ({ } diff --git a/apps/readest-app/src/components/settings/CustomDictionaries.tsx b/apps/readest-app/src/components/settings/CustomDictionaries.tsx index 7efa3de3..f6703154 100644 --- a/apps/readest-app/src/components/settings/CustomDictionaries.tsx +++ b/apps/readest-app/src/components/settings/CustomDictionaries.tsx @@ -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 = ({ 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 = ({ 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} diff --git a/apps/readest-app/src/services/dictionaries/systemDictionary.ts b/apps/readest-app/src/services/dictionaries/systemDictionary.ts index d9076459..3164f133 100644 --- a/apps/readest-app/src/services/dictionaries/systemDictionary.ts +++ b/apps/readest-app/src/services/dictionaries/systemDictionary.ts @@ -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 diff --git a/apps/readest-app/src/services/environment.ts b/apps/readest-app/src/services/environment.ts index 8ce2b613..cd0209f2 100644 --- a/apps/readest-app/src/services/environment.ts +++ b/apps/readest-app/src/services/environment.ts @@ -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;