fix(kopiaui): adjust handling of default repository #4561
Conversation
- Only create the default repository if no repositories are configured. - Don't open repository window at startup if repositories are configured. This introduces an additional env variable `KOPIA_CUSTOM_APPDATA` which is used in the tests but could be used in other scenarios as well.
There was a problem hiding this comment.
Pull Request Overview
This PR adjusts the handling of repositories such that a default repository is only created if no repository is configured, and avoids opening the repository window at startup when repositories exist. Key changes include:
- Updating the test suite to use a temporary application data directory and new test hooks.
- Modifying electron.js to set a custom app data path when KOPIA_CUSTOM_APPDATA is provided.
- Changing the logic in config.js to conditionally add a default repository based on the count of configurations.
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| app/tests/main.spec.js | Refactored test setup, added temporary app data dir creation, and enhanced tests for repository window handling and default repository behavior. |
| app/public/electron.js | Added handling for the custom app data directory and exposed the allConfigs function via test hooks. |
| app/public/config.js | Modified default repository creation logic to check if any repository config exists. |
| }); | ||
|
|
||
| if (!configs["repository"]) { | ||
| if (count === 0) { |
There was a problem hiding this comment.
Using a count check for repository configuration might be ambiguous; consider explicitly checking if Object.keys(configs).length is zero for improved clarity.
There was a problem hiding this comment.
@jkowalski I agree with Copilot, that Object.keys(configs).length === 0 would be more readable. I went for count === 0 to keep the changeset minimal, but if you like I can prepend a refactor commit which removes count entirely and instead uses Object.keys(configs).length?
The cleanest solution IMHO would be to use a Map, but personally I would suggest to keep such a refactoring out of a fix pull request.
| function createTemporaryAppDataDir() { | ||
| const tmpDir = fs.mkdtempSync(path.join(os.tmpdir(), 'kopia-test-')); | ||
| fs.mkdirSync(path.join(tmpDir, 'kopia')); |
There was a problem hiding this comment.
[nitpick] Using synchronous filesystem operations in tests can block the event loop; consider using asynchronous versions if these tests begin to negatively affect overall performance.
| function createTemporaryAppDataDir() { | |
| const tmpDir = fs.mkdtempSync(path.join(os.tmpdir(), 'kopia-test-')); | |
| fs.mkdirSync(path.join(tmpDir, 'kopia')); | |
| async function createTemporaryAppDataDir() { | |
| const tmpDir = await fs.promises.mkdtemp(path.join(os.tmpdir(), 'kopia-test-')); | |
| await fs.promises.mkdir(path.join(tmpDir, 'kopia')); |
There was a problem hiding this comment.
For the sake of simplicity I would suggest to stay with the synchronous operations, since it shouldn't be a performance issue.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #4561 +/- ##
==========================================
+ Coverage 75.86% 76.36% +0.49%
==========================================
Files 470 527 +57
Lines 37301 40091 +2790
==========================================
+ Hits 28299 30615 +2316
- Misses 7071 7453 +382
- Partials 1931 2023 +92 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Hi @jkowalski, thank you for looking at this pull request. I updated its name which should now pass the |
|
thanks for the fix! |
tl;dr
This introduces an additional env variable
KOPIA_CUSTOM_APPDATAwhich is used in the tests but could be used in other scenarios as well.Fixes #2649
Hi, first of all thank you and all contributors for your work on this software!
I've been using Kopia for several years now and it has been working great for me.
The only issue I had was that on macOS Kopia would open the repository window of the default and another configured repository upon every restart. I stumbled across issue #2649 and I might be able to provide a fix for both issues.
I tried to mimic the existing code style and while I admit, that the test duration of
main.spec.jsis increased significantly, I hope it's not in a range which is problematic compared to the overall build duration.If there are any issues with the change, please let me know. For me personally, the issue was already fixed by following a suggestion in #2649 but I figured, it would be nice if other users wouldn't have to adjust the configuration manually.
Thanks again and best regards
Andreas