fix(settings): align defaults with PRD §6.10 (#67)#77
Merged
Conversation
Update stale fixture values in 5 test files to match the new backend defaults introduced by issue #67: maxRetries 3→5, maxConcurrentDownloads 3→4, retryDelaySeconds 5→10, webInterfacePort 9666→9876, restApiEnabled false→true, websocketEnabled false→true.
github-actions
Bot
added
documentation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughThis PR aligns runtime defaults with PRD §6.10, injects first-launch defaults for Changes
Sequence Diagram(s)sequenceDiagram
participant Runner as App::run()
participant Fs as resolve_system_download_dir()
participant UUID as Uuid::new_v4()
participant Store as TomlConfigStore::new()
Runner->>Fs: query system Downloads dir
Fs-->>Runner: Option<download_dir>
Runner->>UUID: generate random api_key (UUIDv4)
UUID-->>Runner: api_key String
Runner->>Store: new(config_path, download_dir_opt, Some(api_key))
alt TOML exists
Store->>Store: load persisted AppConfig from disk
else TOML missing
Store->>Store: hydrate AppConfig using injected defaults
end
Store-->>Runner: configured TomlConfigStore
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Greptile SummaryThis PR aligns 9 Confidence Score: 5/5Safe to merge — the prior security concern is fully resolved; only a minor test-fixture inconsistency remains. All P0/P1 findings from the previous review round are addressed: first-launch now enforces a non-empty UUID api_key, the domain gatekeeper (web_interface_enabled: false) is unchanged, and 582 backend + 367 frontend tests pass. The single remaining finding is a P2 style nit in a test fixture. No files require special attention. settingsStore.test.ts has a minor apiKey fixture inconsistency worth cleaning up but it does not affect correctness.
|
| Filename | Overview |
|---|---|
| src-tauri/src/adapters/driven/config/toml_config_store.rs | Adds default_download_dir and default_api_key constructor params, enforces non-empty bootstrap key on first launch, and provides comprehensive new tests covering all first-launch/existing-config edge cases. |
| src-tauri/src/domain/model/config.rs | Defaults aligned to PRD; domain stays pure with gatekeeper web_interface_enabled: false; new test locks all 9 changed defaults and the intentional empty-key invariant. |
| src-tauri/src/lib.rs | Wires resolve_system_download_dir() and a freshly-generated UUIDv4 into TomlConfigStore::new() — correct first-launch bootstrap that is inert on subsequent starts. |
| src-tauri/src/adapters/driven/filesystem/download_dir.rs | Thin dirs::download_dir() adapter returning Option<String>; test guards against None in minimal envs (CI containers). |
| src-tauri/tests/app_state_wiring.rs | Updated to pass a bootstrap key as default_api_key to the new TomlConfigStore::new signature. |
| src/views/SettingsView/tests/Sections.test.tsx | Fixture defaults updated; the restApiEnabled=false override added correctly to the 'should not show API key' test that would otherwise always pass with the new true default. |
| src/stores/tests/settingsStore.test.ts | Numeric defaults updated; apiKey left as empty string while restApiEnabled is now true — minor inconsistency with first-launch behaviour introduced in this PR. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[App startup
TomlConfigStore::new] -->|injects resolve_system_download_dir
+ uuid::Uuid::new_v4| B[get_config called]
B --> C{config.toml
exists?}
C -- No / First launch --> D[read_or_default:
validate default_api_key non-empty]
D -->|empty or None| E[DomainError:
missing bootstrap key]
D -->|valid UUID| F[AppConfig with
download_dir + api_key hydrated]
F --> G[write_config to disk
atomic tmp→rename]
C -- Yes / Existing install --> H[parse TOML via
serde default]
H --> I[AppConfig from file
injected values ignored]
G --> J[Return AppConfig]
I --> J
Prompt To Fix All With AI
This is a comment left during a code review.
Path: src/stores/__tests__/settingsStore.test.ts
Line: 28
Comment:
**`apiKey` inconsistent with new first-launch behaviour**
`baseConfig` now has `restApiEnabled: true` (line 27) and `websocketEnabled: true` (line 30), but `apiKey` remains empty. On a fresh install the adapter injects a non-empty UUID, so this fixture represents a state the bootstrap guard explicitly rejects. If any test asserts on the API-key field visibility while `restApiEnabled` is `true`, a blank key may produce misleading coverage. Consider aligning this to a non-empty placeholder, matching `Sections.test.tsx`.
How can I resolve this? If you propose a fix, please make it concise.Reviews (3): Last reviewed commit: "fix(settings): fail-fast when first-laun..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
CHANGELOG.md (1)
14-14: Consider breaking dense settings list into bullets for readability.The line currently documents 9 distinct default value changes in a single ~370-character sentence, making it difficult to scan. Breaking into sub-bullets would improve readability, especially for users reviewing the changelog to understand what changed.
♻️ Alternative formatting with bullets
-- Default settings values now match PRD §6.10: `autoExtract` on, `maxConcurrentDownloads=4`, `maxRetries=5`, `retryDelaySeconds=10`, `minFileSizeMb=1.0`, `verifyChecksums` on, `webInterfacePort=9876`, REST API and WebSocket enabled by default. `downloadDir` now resolves to the OS default Downloads directory on first launch. (`#67`) +- Default settings values now match PRD §6.10 (fresh installations only; existing config.toml files are not migrated): (`#67`) + - `autoExtract` enabled + - `maxConcurrentDownloads` increased from 3 to 4 + - `maxRetries` increased from 3 to 5 + - `retryDelaySeconds` increased from 5 to 10 + - `minFileSizeMb` increased from 0 to 1.0 + - `verifyChecksums` enabled + - `webInterfacePort` changed from 9666 to 9876 + - REST API and WebSocket enabled by default + - `downloadDir` now resolves to the OS default Downloads directory on first launch🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@CHANGELOG.md` at line 14, Replace the single dense sentence in CHANGELOG.md describing default setting changes with a short lead sentence followed by a bulleted list enumerating each setting and its new default (referencing PRD §6.10), e.g. separate entries for autoExtract, maxConcurrentDownloads=4, maxRetries=5, retryDelaySeconds=10, minFileSizeMb=1.0, verifyChecksums, webInterfacePort=9876, REST API and WebSocket enabled, and downloadDir behavior (resolves to OS Downloads on first launch) so each change is easy to scan.src-tauri/tests/app_state_wiring.rs (1)
44-47: Consider adding a companion test for the injected default-download-dir path.Line 44–47 validates the new signature with
None, but it doesn’t cover the new behavior branch where a default directory is injected. A small integration test usingSome(...)would protect that path from regressions.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src-tauri/tests/app_state_wiring.rs` around lines 44 - 47, Add a new test that asserts TomlConfigStore::new handles the injected default-download-dir by calling TomlConfigStore::new with Some(path) instead of None, creating the Arc<dyn ConfigStore> (same variable name config_store) and verifying the stored/default download directory value is set to the provided path; locate the constructor usage in the existing test (TomlConfigStore::new) and mirror the setup (use config_dir.path().join(...) for a temporary dir) but pass Some(default_dir) and add assertions that the ConfigStore API (the method that returns the download-dir) returns that injected path to protect that branch from regressions.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@CHANGELOG.md`:
- Line 14: Update the CHANGELOG entry that starts "Default settings values now
match PRD §6.10" to explicitly state that these default value changes apply only
to fresh installations and that existing config.toml files will not be migrated
or modified; add a clarifying sentence such as "Existing installations and
config.toml files are left unchanged — these defaults apply only on first launch
/ fresh installs" immediately after the current description so readers know the
change does not alter user configs on upgrade.
In `@src-tauri/src/adapters/driven/filesystem/download_dir.rs`:
- Around line 22-30: The test resolves_some_path_on_supported_platforms is too
strict by asserting result.is_some() for resolve_system_download_dir (which
wraps dirs::download_dir()) — update the test to tolerate None: if
resolve_system_download_dir() returns Some(path) assert the path is not empty,
otherwise treat it as an acceptable skip (do not fail the test), optionally
emitting a message that the download dir is undiscoverable in this environment;
ensure you reference resolves_some_path_on_supported_platforms and
resolve_system_download_dir when making the change.
In `@src-tauri/src/domain/model/config.rs`:
- Around line 203-206: Add assertions to the regression test that verify the
security gate defaults to prevent accidental exposure: after the existing remote
access checks (where web_interface_port, rest_api_enabled, websocket_enabled are
asserted), assert that config.web_interface_enabled is false and that
config.api_key is empty (equal to an empty string) so the gatekeeper defaults
are locked down.
---
Nitpick comments:
In `@CHANGELOG.md`:
- Line 14: Replace the single dense sentence in CHANGELOG.md describing default
setting changes with a short lead sentence followed by a bulleted list
enumerating each setting and its new default (referencing PRD §6.10), e.g.
separate entries for autoExtract, maxConcurrentDownloads=4, maxRetries=5,
retryDelaySeconds=10, minFileSizeMb=1.0, verifyChecksums, webInterfacePort=9876,
REST API and WebSocket enabled, and downloadDir behavior (resolves to OS
Downloads on first launch) so each change is easy to scan.
In `@src-tauri/tests/app_state_wiring.rs`:
- Around line 44-47: Add a new test that asserts TomlConfigStore::new handles
the injected default-download-dir by calling TomlConfigStore::new with
Some(path) instead of None, creating the Arc<dyn ConfigStore> (same variable
name config_store) and verifying the stored/default download directory value is
set to the provided path; locate the constructor usage in the existing test
(TomlConfigStore::new) and mirror the setup (use config_dir.path().join(...) for
a temporary dir) but pass Some(default_dir) and add assertions that the
ConfigStore API (the method that returns the download-dir) returns that injected
path to protect that branch from regressions.
🪄 Autofix (Beta)
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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 61330c09-0fc9-4c88-b1bc-a8972af7e8fd
📒 Files selected for processing (18)
CHANGELOG.mdsrc-tauri/src/adapters/driven/config/toml_config_store.rssrc-tauri/src/adapters/driven/filesystem/download_dir.rssrc-tauri/src/adapters/driven/filesystem/mod.rssrc-tauri/src/domain/model/config.rssrc-tauri/src/domain/ports/driven/tests.rssrc-tauri/src/lib.rssrc-tauri/tests/app_state_wiring.rssrc/components/__tests__/ClipboardIndicator.test.tsxsrc/hooks/__tests__/useAppEffects.test.tssrc/hooks/__tests__/useDownloadDetail.test.tssrc/layouts/__tests__/AppLayout.test.tsxsrc/stores/__tests__/settingsStore.test.tssrc/views/DownloadDetailsPanel/__tests__/DownloadDetailsPanel.test.tsxsrc/views/DownloadDetailsPanel/__tests__/FileInfoSection.test.tsxsrc/views/DownloadDetailsPanel/__tests__/MetricsSection.test.tsxsrc/views/SettingsView/__tests__/Sections.test.tsxsrc/views/SettingsView/__tests__/SettingsView.test.tsx
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Address PR review: - greptile (P1 security): hydrate api_key with a random uuid on first launch so REST/WS protocols never start with an empty credential. TomlConfigStore::new now takes a third Option<String> for the default key; lib.rs injects uuid::Uuid::new_v4() at bootstrap, tests pass None. - coderabbit (CHANGELOG): clarify the change applies to fresh installs only; existing config.toml files are left untouched. - coderabbit (download_dir test): tolerate None resolution on minimal environments instead of asserting is_some() unconditionally. - coderabbit (snapshot test): lock web_interface_enabled=false and api_key empty in the bare-domain defaults to prevent accidental exposure regressions.
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 625b7ce8-5604-4e55-8e04-0c89afcd5b50
📒 Files selected for processing (6)
CHANGELOG.mdsrc-tauri/src/adapters/driven/config/toml_config_store.rssrc-tauri/src/adapters/driven/filesystem/download_dir.rssrc-tauri/src/domain/model/config.rssrc-tauri/src/lib.rssrc-tauri/tests/app_state_wiring.rs
✅ Files skipped from review due to trivial changes (1)
- src-tauri/src/domain/model/config.rs
🚧 Files skipped from review as they are similar to previous changes (4)
- src-tauri/tests/app_state_wiring.rs
- src-tauri/src/lib.rs
- src-tauri/src/adapters/driven/filesystem/download_dir.rs
- CHANGELOG.md
#67) coderabbit MAJOR: the unwrap_or_else fallback in read_or_default still produced an empty api_key when default_api_key was None or whitespace, defeating the bootstrap guarantee. Replace the fallback with a StorageError so a misconfigured caller cannot silently ship an unauthenticated REST/WebSocket server. Tests that don't assert on the key now pass a constant bootstrap value; two new tests lock the None and whitespace paths to the error branch.
This was referenced Apr 25, 2026
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.
Summary
AppConfigdefaults with PRD §6.10:autoExtract,maxConcurrentDownloads=4,maxRetries=5,retryDelaySeconds=10,verifyChecksums,webInterfacePort=9876,restApiEnabled,websocketEnabled,minFileSizeMb=1.0.resolve_system_download_dir()adapter wrappingdirs::download_dir()sodownloadDirresolves cross-platform on first launch (Linux/macOS/Windows). Domain stays pure.web_interface_enabled/rest_api_enabled/api_keysecurity triad in the domain model.config.tomlfiles are preserved untouched (only fresh installs get the new defaults).Closes #67.
Test plan
Summary by cubic
Aligns
AppConfigdefaults with PRD §6.10. On first launch we setdownloadDirto the OS Downloads folder and generate a UUIDv4apiKey(fail-fast if missing) so REST/WS never start unauthenticated; existing configs stay unchanged.New Features
autoExtracton,maxConcurrentDownloads=4,maxRetries=5,retryDelaySeconds=10,verifyChecksumson,minFileSizeMb=1.0,webInterfacePort=9876,restApiEnabledon,websocketEnabledon.resolve_system_download_dir()(wrapsdirs) to setdownloadDircross‑platform on first launch only.apiKey; we now fail fast if the bootstrap key is missing/blank to avoid starting REST/WebSocket without auth.Migration
config.tomlfiles are preserved; new defaults apply to fresh installs only.Written for commit ac0d017. Summary will update on new commits.
Summary by CodeRabbit