fix(portable): macOS webview profile cleanup & UI notice (Addresses #227) - #267
Conversation
tonhowtf
left a comment
There was a problem hiding this comment.
Thanks for taking a swing at #227 — the write-up is thorough and I can tell you read the issue. But I have to request changes, and it's not cosmetic: the core mechanism here can destroy user data, and I don't think a cleanup-based approach can ever deliver what portable mode promises. Let me walk through why.
1. This deletes the installed app's data. On macOS, ~/Library/Application Support/wtf.tonho.omniget is not "WebView session data" — it's OmniGet's primary data directory for a normal (non-portable) install: settings.json, the SQLite history DB, cookie storage, plugins/, bin/. The actual WKWebView data lives under ~/Library/WebKit/<bundle-id> and Caches. So the concrete failure mode is: a user has OmniGet installed on their Mac, runs a portable copy from a USB stick once, and cleanup_macos_portable_webview_dirs() wipes their installed app's settings, history, cookies and plugins — at startup, before a window even opens, and again at exit. The cleanup has no way to tell "portable session residue" apart from "the installed app's data", because on macOS they're the same paths. That alone is a blocker.
2. It also breaks the legacy settings adoption. The startup call was inserted in check_portable_mode() before the block that adopts settings.json from the OS profile (main.rs, the "Older versions resolved the settings store…" block). You delete the file, then three lines later the existing code tries to copy it.
3. Write-then-delete doesn't fulfill the portable promise, even setting (1) aside. RunEvent::ExitRequested never fires on crash, SIGKILL or force-quit — and the startup "crash recovery" only helps if the portable app runs again on the same machine, which is precisely what the USB-stick user doesn't do. That's the central use case (leave no trace on a borrowed machine) failing in exactly the scenario it exists for. Even on graceful exit, WebKit's network process flushes cookies/cache during teardown and can recreate files after your remove_dir_all runs. And deletion isn't erasure — the bytes were on disk (APFS snapshots, Time Machine). The promise is "never write to the profile"; "write, then erase" is a weaker promise, and the UI copy ("automatically cleaned up when OmniGet exits") states it as stronger than it is.
4. The CI green worries me most. The smoke test asserts after exit, and the smoke exit path is graceful (OMNIGET_SMOKE_EXIT_MS → handle.exit(0) → ExitRequested → your cleanup runs) — so adding macos-latest passes not because the leak is fixed, but because the assertion's meaning got inverted from "never wrote to the profile" to "erased it before we looked". That test failing on macOS is the honest state until wry exposes WKWebsiteDataStore configuration. I'd rather have no macOS smoke job than a green one attesting something false. (The unexplained NODE_OPTIONS heap bump in that job should go too, or be its own PR with a reason.)
5. Scope/size. 494 of the 716 added lines are AI planning documents under docs/superpowers/ — please don't commit those. The ContextHint.svelte API change is dead code (the final SettingsStorage.svelte doesn't use the component, and the new type prop isn't used even inside it). 7 of 10 locales received raw English strings. The it/zh-TW wording fixes are fine but belong in their own PR, and the new "Storage" settings category collides with an in-progress settings-IA rework.
What I'd gladly take from this PR: the honest half. Issue #227's option 2 — get_portable_info + a notice in settings that says the webview is not portable on macOS, full stop, no cleanup, no promise of erasure — is exactly the kind of transparency I asked for, and your IPC command and UI wiring are most of the way there. If you slim this down to that (~80 lines: command, notice, en strings + scripts/sync-locales.mjs, no CI change, no docs, no ContextHint change), I'll review it quickly and happily merge. The real fix for the leak itself has to happen upstream in wry (WKWebsiteDataStore with a custom location); if you want to open that upstream issue and link it from ours, that would be genuinely useful too.
Addresses tonhowtf#227 with issue option 2: transparency, not cleanup. Add a `get_portable_info` command and surface it in the existing Storage settings section. When portable mode is on, the panel names the data directory in use; on macOS it also states that the WebView keeps its session data under ~/Library and that portable mode cannot redirect it. The real fix belongs upstream in wry, which does not read `data_directory` on WKWebView the way it does for WebKitGTK. Until that exists, telling the user the truth beats letting them assume the USB stick left no trace. Non-en locales carry the English copy for now: check:i18n --strict aborts when a locale is missing a key en.json has, so the keys have to land in all ten files at once.
6c28ac6 to
2b7c4e0
Compare
|
Sorry — you were right on every count, and the first point is the one I feel worst about. I shipped a You were also right that the green CI was the worst part, not the best. The smoke assertion ran after a graceful I've force-pushed a rewrite (
Everything else is gone: the cleanup helper and its test, both call sites, the The PR is now 14 files and 126 lines, down from 23 and 716. Verification: The 107 warnings are the pre-existing ones under One deliberate deviation from your ~80-line sketch that I should flag rather than have you find: the notice strings go into all ten locale files in English, not just On the upstream fix: agreed that this belongs in wry via Thanks for the detailed review — the failure mode in point 1 is one I'd rather learn from a review than from a bug report. |
|
Merged. This is exactly the honest version the issue needed — the notice says what's true, nothing more, and 126 lines instead of 716. Owning the |
Addresses #227
Summary
On macOS, WebKit/Wry creates WebView session data in
~/Library/Application Support/wtf.tonho.omniget,~/Library/Caches/wtf.tonho.omniget, and~/Library/WebKit/wtf.tonho.omnigeteven whenOMNIGET_PORTABLE=1is set, because WKWebView hardcodes these system directories.This PR provides a hybrid mitigation on macOS:
OMNIGET_PORTABLE=1), the app automatically cleans up created WebView profile/cache directories under~/Libraryboth at app startup (for crash recovery) and upon exit.Key Changes
src-tauri/src/core/portable.rs):cleanup_macos_portable_webview_dirs()to purge~/Library/Application Support/wtf.tonho.omniget,~/Library/Caches/wtf.tonho.omniget, and~/Library/WebKit/wtf.tonho.omniget.cleanup_removes_target_dirscovering path cleanup logic.src-tauri/src/main.rs,src-tauri/src/lib.rs):check_portable_mode().RunEvent::ExitRequested.src-tauri/src/commands/app_lifecycle.rs):get_portable_infoIPC command returningis_portable,data_dir, andmacos_webview_noticeflag.src/routes/settings/+page.svelte,src/components/settings/SettingsStorage.svelte):src/lib/i18n/*.json):portable_label,portable_active,portable_macos_notice,cat_storage) across all 10 locales using proper{{dir}}interpolation syntax..github/workflows/ci.yml):macos-latestto the portable mode smoke test matrix.Verification
cargo test --manifest-path src-tauri/Cargo.toml): 344/344 passing.pnpm check && pnpm test && pnpm check:i18n): 0 errors, all 10 locales in sync.OMNIGET_PORTABLE=1 OMNIGET_DATA_DIR=$(pwd)/data cargo tauri dev.