feat(firstrun): let the user choose the portable folder (#766) - #1404
Merged
Conversation
Portable mode put everything in `OmniVoiceStudio-Data` beside the app, and
that was the only storage row on the setup screen you could look at but not
change — installed mode had a picker for all three of its directories.
The pin had a real reason. `portable_base()` is computed from the
executable's location and never read from config, which is exactly what
makes a portable install self-discovering: plug the drive into another
machine and the app finds its data with no per-machine state. A
user-chosen folder breaks that, because the only record of the location
would live inside the folder being located.
So record it somewhere findable, in this order:
1. `portable.path` beside the app. This keeps portability — it travels
with the app and needs nothing from the host.
2. `portableDir` in the per-user config, for app folders that are not
writable (`/Applications`, `Program Files`) where no pointer can be
written. Machine-bound by nature, which is why it is second.
3. `<anchor>/OmniVoiceStudio-Data` — the historical default. Existing
portable installs have neither a pointer nor a config field, so they
resolve byte-identically to before.
Step 2 has a trap worth naming: `config_path` → `portable_config_file` →
`portable_base` → step 2 would recurse forever if it went through
`load_config`. It reads the platform config file directly instead, and can
therefore never consult the portable one.
This also retires #766. A default Program Files install no longer greys
Portable out — you point it at a writable disk and get the machine-bound
variant, and the setup screen says which of the two you are getting rather
than quietly downgrading the promise.
Tests: one Rust test covering the whole resolution order (default,
pointer, padded pointer, empty pointer, plan override, blank override) —
single fn because it mutates APPIMAGE, which is process-global. Four
frontend tests pinning the JS↔Rust payload key and both captions; three
fail before.
Frontend 1707 passed, Rust 108 passed. All 21 locales updated in lockstep;
docs/install/windows.md rewritten for the new behaviour.
Contributor
📝 WalkthroughWalkthroughPortable mode now supports a user-selected storage folder. The backend resolves relocated portable paths through pointer files and per-user configuration. First-run setup validates and submits the selected path, displays relocation status, and includes localized messages and tests. ChangesPortable installation flow
Estimated code review effort: 4 (Complex) | ~45 minutes 🚥 Pre-merge checks | ✅ 8 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (8 passed)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint install failed. For unrecoverable errors, disable the tool in CodeRabbit configuration. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
frontend/src/components/FirstRunSetup.jsx (1)
322-322: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winReset the target result when
portableDirchanges.When Line 322 receives a new path,
useTargetCheckretains the previous path result during its 250 ms debounce. The Start button can remain enabled for a folder that has not passed its own writable and space check. As per path instructions, clearcheckbefore scheduling the new probe so the loading gate blocks Start.Proposed fix
useEffect(() => { + setCheck(null); if (!path) { - setCheck(null); return; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/src/components/FirstRunSetup.jsx` at line 322, Update the useTargetCheck hook used by portableCheck so its check result is cleared immediately whenever the target path changes, before the 250 ms probe is scheduled. Ensure the loading state blocks Start until the new path completes its writable and space validation, while preserving the existing debounce behavior.Source: Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/install/windows.md`:
- Around line 123-125: Update the Portable setup description near the first-run
setup instructions to state that the adjacent OmniVoiceStudio-Data folder is the
default location, while retaining that users can choose any writable folder
through Change…. Do not describe the adjacent folder as the only portable
location.
In `@frontend/src-tauri/src/setup.rs`:
- Around line 223-233: Update clear_portable_dir to load the machine-local
configuration from config::config_path_for_machine() rather than
config::load_config(app), and clear portable_dir there unconditionally before
saving it back. Preserve the existing pointer removal and ensure the machine
config record is updated even when the currently loaded relocated config has no
portable_dir.
- Around line 198-202: Absolute paths written by portable_pointer_path in
setup.rs are not relocatable, so do not claim unsupported cross-machine
discovery. Update frontend/src-tauri/src/setup.rs:198-202 to persist a
relocatable reference or otherwise constrain the portability behavior; revise
docs/install/windows.md:130-137 and the corresponding portability claims in
frontend/src/i18n/locales/tr.json:1972, uk.json:1972, vi.json:1972,
zh-CN.json:1979, and zh-TW.json:1972 to remove the unconditional cross-machine
guarantee.
---
Outside diff comments:
In `@frontend/src/components/FirstRunSetup.jsx`:
- Line 322: Update the useTargetCheck hook used by portableCheck so its check
result is cleared immediately whenever the target path changes, before the 250
ms probe is scheduled. Ensure the loading state blocks Start until the new path
completes its writable and space validation, while preserving the existing
debounce behavior.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 720253dc-133c-41f0-91fd-3f9dca5bdabb
📒 Files selected for processing (27)
CHANGELOG.mddocs/install/windows.mdfrontend/src-tauri/src/config.rsfrontend/src-tauri/src/setup.rsfrontend/src/components/FirstRunSetup.jsxfrontend/src/i18n/locales/ar.jsonfrontend/src/i18n/locales/de.jsonfrontend/src/i18n/locales/en.jsonfrontend/src/i18n/locales/es.jsonfrontend/src/i18n/locales/fr.jsonfrontend/src/i18n/locales/hi.jsonfrontend/src/i18n/locales/id.jsonfrontend/src/i18n/locales/it.jsonfrontend/src/i18n/locales/ja.jsonfrontend/src/i18n/locales/ko.jsonfrontend/src/i18n/locales/nl.jsonfrontend/src/i18n/locales/pl.jsonfrontend/src/i18n/locales/pt.jsonfrontend/src/i18n/locales/ru.jsonfrontend/src/i18n/locales/sv.jsonfrontend/src/i18n/locales/th.jsonfrontend/src/i18n/locales/tr.jsonfrontend/src/i18n/locales/uk.jsonfrontend/src/i18n/locales/vi.jsonfrontend/src/i18n/locales/zh-CN.jsonfrontend/src/i18n/locales/zh-TW.jsonfrontend/src/test/FirstRunSetupPortableDir.test.jsx
Both Major findings from CodeRabbit on #1404 are real, and both were mine. An absolute pointer does not survive the mount path changing. I stored `/Volumes/Stick/Data` and told the user the install "still finds itself" on another machine — where that path is `E:\Data` and resolves to nothing. The claim was false in exactly the case portable mode exists for. A folder INSIDE the app's own directory is now recorded RELATIVE to it, so app + folder move as a unit and the absolute path is free to change. That is the USB-stick case, and the only one where the promise holds. Anywhere else there is nothing relocatable to store, so the absolute path is kept and the UI says the install is tied to it. Three honest states now instead of one overclaim; `pointer_payload` is pure so the decision that governs the promise is testable without an AppHandle. `clear_portable_dir` did not clear. It called `load_config`, which resolves via `config_path` → `portable_config_file` → `portable_base` → the very `portableDir` being erased, loading the RELOCATED config whose own `portable_dir` is None. The `is_some()` guard then skipped, the machine record survived, and the old folder kept winning after the user picked the default. It reads and writes the machine config directly now. Also corrected the Windows doc, which still described the folder as fixed beside the executable two paragraphs above the picker that moves it. Rust 109 passed, frontend 1705 passed. All 21 locales retranslated — both strings changed meaning, so leaving the old wording would have shipped the retracted promise in 20 languages.
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
frontend/src-tauri/src/setup.rs (1)
249-256: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winLoad the machine configuration directly before recording relocation.
Line 249 can load the existing portable configuration, then Lines 251-256 overwrite machine-local settings with that old portable configuration when a prior relocation exists. Load and update
config_path_for_machine()directly, then add a regression test with an existing relocatedconfig.json; as per coding guidelines, “Fix the root cause with a fail-before/pass-after regression test and the smallest correct change.”🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/src-tauri/src/setup.rs` around lines 249 - 256, Update the relocation logic around config::load_config and config::save_config_at to load the existing machine-specific configuration from config_path_for_machine() directly before setting portable_dir, rather than loading the portable configuration. Preserve the existing directory creation and save behavior, and add a regression test covering an already-relocated config.json that fails before the change and passes afterward.Source: Coding guidelines
🧹 Nitpick comments (1)
frontend/src/test/FirstRunSetupPortableDir.test.jsx (1)
130-158: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd Windows portability cases.
At Lines 133-153, every relocation case uses the shared
os: 'macos'fixture and POSIX paths, so a Windows drive-letter or separator regression can pass while the Windows portable flow fails; the repository lists Windows as a supported desktop target. (github.com) Parameterizeos,anchorDir, andbaseDir, then add app-contained, external, and read-only Windows cases. As per coding guidelines, “Keep default behavior identical on macOS, Windows, and Linux.”🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/src/test/FirstRunSetupPortableDir.test.jsx` around lines 130 - 158, Extend the relocation portability tests around the existing cases to cover Windows alongside macOS, parameterizing os, anchorDir, and baseDir with Windows drive-letter and separator paths. Add Windows app-contained, external, and read-only scenarios, preserving the existing expectations and default behavior for macOS, Windows, and Linux.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@frontend/src-tauri/src/setup.rs`:
- Around line 249-256: Update the relocation logic around config::load_config
and config::save_config_at to load the existing machine-specific configuration
from config_path_for_machine() directly before setting portable_dir, rather than
loading the portable configuration. Preserve the existing directory creation and
save behavior, and add a regression test covering an already-relocated
config.json that fails before the change and passes afterward.
---
Nitpick comments:
In `@frontend/src/test/FirstRunSetupPortableDir.test.jsx`:
- Around line 130-158: Extend the relocation portability tests around the
existing cases to cover Windows alongside macOS, parameterizing os, anchorDir,
and baseDir with Windows drive-letter and separator paths. Add Windows
app-contained, external, and read-only scenarios, preserving the existing
expectations and default behavior for macOS, Windows, and Linux.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 331935ac-732a-4c25-9cd2-34846f7bcf05
📒 Files selected for processing (25)
docs/install/windows.mdfrontend/src-tauri/src/setup.rsfrontend/src/components/FirstRunSetup.jsxfrontend/src/i18n/locales/ar.jsonfrontend/src/i18n/locales/de.jsonfrontend/src/i18n/locales/en.jsonfrontend/src/i18n/locales/es.jsonfrontend/src/i18n/locales/fr.jsonfrontend/src/i18n/locales/hi.jsonfrontend/src/i18n/locales/id.jsonfrontend/src/i18n/locales/it.jsonfrontend/src/i18n/locales/ja.jsonfrontend/src/i18n/locales/ko.jsonfrontend/src/i18n/locales/nl.jsonfrontend/src/i18n/locales/pl.jsonfrontend/src/i18n/locales/pt.jsonfrontend/src/i18n/locales/ru.jsonfrontend/src/i18n/locales/sv.jsonfrontend/src/i18n/locales/th.jsonfrontend/src/i18n/locales/tr.jsonfrontend/src/i18n/locales/uk.jsonfrontend/src/i18n/locales/vi.jsonfrontend/src/i18n/locales/zh-CN.jsonfrontend/src/i18n/locales/zh-TW.jsonfrontend/src/test/FirstRunSetupPortableDir.test.jsx
🚧 Files skipped from review as they are similar to previous changes (23)
- frontend/src/i18n/locales/es.json
- frontend/src/i18n/locales/sv.json
- frontend/src/i18n/locales/zh-TW.json
- frontend/src/i18n/locales/ko.json
- frontend/src/i18n/locales/en.json
- frontend/src/i18n/locales/vi.json
- frontend/src/i18n/locales/pt.json
- frontend/src/i18n/locales/zh-CN.json
- frontend/src/i18n/locales/uk.json
- frontend/src/i18n/locales/de.json
- frontend/src/i18n/locales/ja.json
- frontend/src/i18n/locales/pl.json
- frontend/src/i18n/locales/hi.json
- frontend/src/components/FirstRunSetup.jsx
- frontend/src/i18n/locales/tr.json
- frontend/src/i18n/locales/th.json
- frontend/src/i18n/locales/it.json
- frontend/src/i18n/locales/ru.json
- frontend/src/i18n/locales/nl.json
- frontend/src/i18n/locales/fr.json
- frontend/src/i18n/locales/id.json
- frontend/src/i18n/locales/ar.json
- docs/install/windows.md
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #766.
Portable mode put everything in
OmniVoiceStudio-Databeside the app — the only storage row on the setup screen you could look at but not change. Installed mode had a picker for all three of its directories.Why it was pinned
portable_base()is computed from the executable's location and never read from config. That is exactly what makes a portable install self-discovering: plug the drive into another machine and the app finds its data with no per-machine state. A user-chosen folder breaks it, because the only record of the location would live inside the folder being located.Resolution order
portable.pathbeside the appportableDirin the per-user config<anchor>/OmniVoiceStudio-DataStep 3 keeps existing portable installs resolving byte-identically: they have neither a pointer nor a config field.
Step 2 has a trap worth naming —
config_path→portable_config_file→portable_base→ step 2 would recurse forever throughload_config. It reads the platform config file directly instead, so it can never consult the portable one.This retires #766
A default
Program Filesinstall no longer greys Portable out. You point it at a writable disk and get the machine-bound variant — and the setup screen says which of the two you're getting rather than quietly downgrading the promise:Tests
""and root the install at/), plan override, blank override. Single fn because it mutatesAPPIMAGE, which is process-global, matching the existing precedent in that file.portableDir→InstallPlan::portable_dir— rename either side and the folder is silently ignored) and both captions. Three fail before.Frontend 1707 passed, Rust 108 passed, typecheck/format/lint clean. All 21 locales updated in lockstep;
docs/install/windows.mdrewritten for the new behaviour.Portable installations can now use a user-selected data folder, with marker-file, per-user, and historical fallback resolution. This enables portable mode for read-only locations such as Program Files and explains whether the selected folder is portable across machines or machine-bound. Rust and frontend tests cover path precedence, folder validation, persistence, and setup messaging; reviewers should verify migration and fallback behavior for existing installations.