Desktop, Mobile: Fixes #14506: Add the ability to delete the default profile#15153
Desktop, Mobile: Fixes #14506: Add the ability to delete the default profile#15153mrjo118 wants to merge 8 commits intolaurent22:devfrom
Conversation
coderabbitai
Bot
added
bug
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/app-desktop/gui/ProfileEditor.tsx (1)
147-151:⚠️ Potential issue | 🟡 MinorConfirmation message does not reflect the default-profile "reset" semantics.
The same prompt — "Delete profile "%s"?\n\nAll data, including notes, notebooks and tags will be permanently deleted." — is shown for both sub-profile deletion and default-profile reset, but for the default profile the entry is kept and only selected data is cleared. Users may be surprised that the profile still appears afterwards, and the success popup "The default profile has been reset." conflicts with the earlier "delete" wording. Consider branching the confirmation message on
isSubProfile(profile)so the default-profile path uses "reset" language consistently.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/app-desktop/gui/ProfileEditor.tsx` around lines 147 - 151, The confirmation uses the same "Delete profile" text for both sub-profiles and the default profile; update the logic around the bridge().showConfirmMessageBox call to branch on isSubProfile(profile) and show different language: for sub-profiles keep the current delete wording with profile.name, and for the default profile use "Reset default profile" / "Reset profile \"%s\"? ... Selected data will be cleared and the profile entry will remain." Ensure the buttons and defaultId remain appropriate and that the subsequent success popup still matches the chosen action (e.g., "The default profile has been reset." when reset path is taken).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/app-desktop/gui/ProfileEditor.tsx`:
- Around line 170-199: The reset currently only removes
dirsToDelete/filesToDelete and omits per-profile items (plugin-data, custom CSS
and keymap) and shows an error dialog for each failed deletion; update the
deletion logic in ProfileEditor.tsx to also delete the profile pluginDataDir
(plugin-data/), the custom CSS filenames from Settings.customCssFilenames (e.g.
userchrome.css, userstyle.css) and keymap-desktop.json (referencing the same
name used in app.ts), and aggregate any deletion errors into a single report
instead of calling bridge().showErrorMessageBox() inside each loop; keep using
shim.fsDriver().exists/ remove/ unlink but collect failures into an array, log
them with logger.error, and show one combined error dialog (or silent log as
mobile deleteProfile does) after both directory and file loops complete.
In `@packages/app-mobile/components/ProfileSwitcher/utils/deleteProfile.ts`:
- Around line 52-73: The current loop in deleteProfile.ts reads resourcesDir
with shim.fsDriver().readDirStats and attempts unlink via shim.fsDriver().unlink
but only logs failures (logger.error) causing the function to report success
even if deletions fail; change this to track per-file outcomes (e.g., count
successes and collect failed file paths/errors), and after the loop return or
throw an error (or surface a detailed failure message) if any unlink operations
failed so the caller/user is not misled by "The default profile has been reset."
Only log individual failures but also propagate an aggregate failure result.
Also tighten the filename check currently using /^[a-z0-9]{32}\./ by restricting
it to the actual Joplin hex-id pattern and allowed suffixes (for example use
[a-f0-9]{32}(?:\.crypted)? or explicitly validate the file extension) to avoid
accidental matches; update the regex used where fileName is tested. Ensure
references to resourcesDir, shim.fsDriver().readDirStats,
shim.fsDriver().unlink, logger.info and logger.error are preserved when
implementing these changes.
---
Outside diff comments:
In `@packages/app-desktop/gui/ProfileEditor.tsx`:
- Around line 147-151: The confirmation uses the same "Delete profile" text for
both sub-profiles and the default profile; update the logic around the
bridge().showConfirmMessageBox call to branch on isSubProfile(profile) and show
different language: for sub-profiles keep the current delete wording with
profile.name, and for the default profile use "Reset default profile" / "Reset
profile \"%s\"? ... Selected data will be cleared and the profile entry will
remain." Ensure the buttons and defaultId remain appropriate and that the
subsequent success popup still matches the chosen action (e.g., "The default
profile has been reset." when reset path is taken).
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: db038f70-eb25-4b72-b811-3c5378e400dc
📒 Files selected for processing (4)
packages/app-desktop/gui/ProfileEditor.tsxpackages/app-mobile/components/ProfileSwitcher/utils/deleteProfile.test.tspackages/app-mobile/components/ProfileSwitcher/utils/deleteProfile.tspackages/lib/services/profileConfig/index.ts
💤 Files with no reviewable changes (2)
- packages/lib/services/profileConfig/index.ts
- packages/app-mobile/components/ProfileSwitcher/utils/deleteProfile.test.ts
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)
packages/app-desktop/gui/ProfileEditor.tsx (1)
101-112:⚠️ Potential issue | 🟠 MajorPersist the profile removal before deleting its directory.
If Line 161 succeeds but Line 168 fails inside
saveNewProfileConfig, the profile remains inprofiles.jsonwhile its data directory is gone. Save the config first and only continue with destructive deletion when that succeeds.🐛 Proposed fix
- const saveNewProfileConfig = async (makeNewProfileConfig: ()=> ProfileConfig) => { + const saveNewProfileConfig = async (makeNewProfileConfig: ()=> ProfileConfig): Promise<boolean> => { try { const newProfileConfig = makeNewProfileConfig(); await saveProfileConfig(`${Setting.value('rootProfileDir')}/profiles.json`, newProfileConfig); dispatch({ type: 'PROFILE_CONFIG_SET', value: newProfileConfig, }); + return true; } catch (error) { logger.error(error); bridge().showErrorMessageBox(error.message); + return false; } };if (subProfile) { const profileDir = `${rootDir}/profile-${profile.id}`; + if (!await saveNewProfileConfig(() => deleteProfileById(profileConfig, profile.id))) return; + try { await shim.fsDriver().remove(profileDir); logger.info('Deleted profile directory: ', profileDir); } catch (error) { logger.error('Error deleting profile directory: ', error); bridge().showErrorMessageBox(error.message); } - - await saveNewProfileConfig(() => deleteProfileById(profileConfig, profile.id)); } else {Also applies to: 160-168
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/app-desktop/gui/ProfileEditor.tsx` around lines 101 - 112, saveNewProfileConfig currently performs destructive deletion before guaranteeing the updated profiles.json is persisted; change the flow so you persist the new ProfileConfig first (using saveProfileConfig inside saveNewProfileConfig and then dispatch the 'PROFILE_CONFIG_SET' action) and only after that await the destructive removal of the profile directory (the deletion code currently at lines ~160-168). In practice: call saveNewProfileConfig (which must complete the save and dispatch) before invoking the directory deletion routine (fs.rm/rimraf or whatever deletion helper you use), and if the deletion fails handle/log/showErrorMessageBox without rolling back the saved profiles.json.
♻️ Duplicate comments (1)
packages/app-desktop/gui/ProfileEditor.tsx (1)
170-201:⚠️ Potential issue | 🟠 MajorDefault reset still leaves some profile data behind and can show a false success message.
keymap-desktop.jsonis now covered, butplugin-data/and custom CSS files still persist after reset. Also, failed deletions at Lines 183 and 197 are followed by the success message at Line 201.🐛 Proposed fix
- const dirsToDelete = ['cache', 'JoplinBackup', 'resources', 'tmp']; - const filesToDelete = ['database.sqlite', 'log.txt', 'settings.json', 'keymap-desktop.json']; + const dirsToDelete = ['cache', 'JoplinBackup', 'resources', 'tmp', 'plugin-data']; + const filesToDelete = ['database.sqlite', 'log.txt', 'settings.json', 'keymap-desktop.json', ...Setting.customCssFilenames]; + const deletionErrors: string[] = []; // Delete directories for (const dir of dirsToDelete) { const fullPath = `${rootDir}/${dir}`; try { if (await shim.fsDriver().exists(fullPath)) { await shim.fsDriver().remove(fullPath); logger.info('Deleted directory: ', fullPath); } } catch (error) { logger.error('Error deleting directory: ', fullPath, error); - bridge().showErrorMessageBox(error.message); + const message = error instanceof Error ? error.message : String(error); + deletionErrors.push(`${fullPath}: ${message}`); } } // Delete files for (const file of filesToDelete) { const fullPath = `${rootDir}/${file}`; try { if (await shim.fsDriver().exists(fullPath)) { await shim.fsDriver().unlink(fullPath); logger.info('Deleted file: ', fullPath); } } catch (error) { logger.error('Error deleting file: ', fullPath, error); - bridge().showErrorMessageBox(error.message); + const message = error instanceof Error ? error.message : String(error); + deletionErrors.push(`${fullPath}: ${message}`); } } + if (deletionErrors.length) { + bridge().showErrorMessageBox(_('Some profile data could not be deleted:\n\n%s', deletionErrors.join('\n'))); + return; + } + bridge().showMessageBox(_('The default profile has been reset.'));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/app-desktop/gui/ProfileEditor.tsx` around lines 170 - 201, The reset currently only targets dirsToDelete and filesToDelete and always shows bridge().showMessageBox('The default profile has been reset.') even if deletions failed; update the cleanup to include the plugin-data directory and common custom CSS filenames (e.g., custom.css, userstyle.css) by adding them to dirsToDelete/filesToDelete (use rootDir + '/plugin-data' and rootDir + '/.config' entries as appropriate), track any deletion errors with a boolean flag or an errors array while using shim.fsDriver().exists/remove/unlink and logger, and only call bridge().showMessageBox(...) when no errors occurred (otherwise call bridge().showErrorMessageBox with the aggregated error messages); refer to the existing variables dirsToDelete, filesToDelete, rootDir, shim.fsDriver(), logger and bridge() to locate and modify the code.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@packages/app-desktop/gui/ProfileEditor.tsx`:
- Around line 101-112: saveNewProfileConfig currently performs destructive
deletion before guaranteeing the updated profiles.json is persisted; change the
flow so you persist the new ProfileConfig first (using saveProfileConfig inside
saveNewProfileConfig and then dispatch the 'PROFILE_CONFIG_SET' action) and only
after that await the destructive removal of the profile directory (the deletion
code currently at lines ~160-168). In practice: call saveNewProfileConfig (which
must complete the save and dispatch) before invoking the directory deletion
routine (fs.rm/rimraf or whatever deletion helper you use), and if the deletion
fails handle/log/showErrorMessageBox without rolling back the saved
profiles.json.
---
Duplicate comments:
In `@packages/app-desktop/gui/ProfileEditor.tsx`:
- Around line 170-201: The reset currently only targets dirsToDelete and
filesToDelete and always shows bridge().showMessageBox('The default profile has
been reset.') even if deletions failed; update the cleanup to include the
plugin-data directory and common custom CSS filenames (e.g., custom.css,
userstyle.css) by adding them to dirsToDelete/filesToDelete (use rootDir +
'/plugin-data' and rootDir + '/.config' entries as appropriate), track any
deletion errors with a boolean flag or an errors array while using
shim.fsDriver().exists/remove/unlink and logger, and only call
bridge().showMessageBox(...) when no errors occurred (otherwise call
bridge().showErrorMessageBox with the aggregated error messages); refer to the
existing variables dirsToDelete, filesToDelete, rootDir, shim.fsDriver(), logger
and bridge() to locate and modify the code.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: d58baad7-5793-4f05-86f2-5b5e8a0ea997
📒 Files selected for processing (1)
packages/app-desktop/gui/ProfileEditor.tsx
| await saveNewProfileConfig(() => deleteProfileById(profileConfig, profile.id)); | ||
| } else { | ||
| const dirsToDelete = ['cache', 'JoplinBackup', 'resources', 'tmp']; | ||
| const filesToDelete = ['database.sqlite', 'log.txt', 'settings.json', 'keymap-desktop.json']; |
There was a problem hiding this comment.
'settings.json'
Some settings are marked with isGlobal: true, which causes these settings to be shared across profiles. Does deleting the toplevel settings.json clear these global settings?
There was a problem hiding this comment.
You are correct, deleting the settings.json of the default profile does clear those settings. I have now pushed up a change to address this
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/lib/models/Setting.ts (1)
1134-1224: Extracted helper has hidden side effects when reused fromresetDefaultProfileSettings.
getFileValuesAndDbUpdateQueriesisn't purely a getter — the inner loop writes secure values to the keychain (lines 1161–1168) and, on keychain failure, adds INSERT queries toqueriesas a fallback. When called fromresetDefaultProfileSettings, thequeriesarray (including the keychain-failure fallback inserts) is discarded, yet the keychain writes still execute using the current profile's cached secure values. In practice this is idempotent and harmless (the active profile can't be the default, so we're just rewriting the running sub-profile's existing keychain entries), but:
- The name suggests pure value/query computation, masking the side effect.
- A future keychain outage during reset would silently drop the DB-fallback path for secure keys (queries are thrown away), which differs from
saveAll's guarantees.Consider splitting the helper so
resetDefaultProfileSettingsreuses only the pure "buildvaluesForFilefrom cache" portion, leaving the keychain/DB side effects insaveAll.Also — this design correctly addresses the earlier question about
isGlobalsettings:splitGlobalAndLocalSettings(valuesForFile)+overwrite: truepreserves shared global settings in the rootsettings.jsonwhile wiping the default profile's local settings. 👍🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/lib/models/Setting.ts` around lines 1134 - 1224, getFileValuesAndDbUpdateQueries performs side effects (writing to keychain and pushing DB fallback queries) which are inappropriate when called from resetDefaultProfileSettings; split the helper into two: a pure getter (e.g., buildValuesForFileFromCache or getValuesForFileOnly) that returns only valuesForFile based on this.cache_ and this.keyStorage/key checks, and a separate method (e.g., getDbUpdateQueriesWithKeychainWrites or applyKeychainAndBuildQueries) that performs the keychain writes and builds the queries; then update saveAll to call both the pure getter and the side-effecting method to get queries, while making resetDefaultProfileSettings call only the pure getter and then use splitGlobalAndLocalSettings as before so no keychain writes or discarded DB queries occur during reset.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/lib/models/settings/FileHandler.ts`:
- Around line 81-87: The overwrite branch in save() stores Setting.schemaUrl
into this.parsedJsonCache_, causing an inconsistent cache shape; update the
overwrite path in FileHandler.save (the block that sets this.parsedJsonCache_)
to omit the '$schema' key before assigning (i.e., spread values but
filter/delete '$schema' or create a new object excluding '$schema'), so
parsedJsonCache_ matches the shape produced by load() and avoids reintroducing
'$schema' during the merge loop in save().
---
Nitpick comments:
In `@packages/lib/models/Setting.ts`:
- Around line 1134-1224: getFileValuesAndDbUpdateQueries performs side effects
(writing to keychain and pushing DB fallback queries) which are inappropriate
when called from resetDefaultProfileSettings; split the helper into two: a pure
getter (e.g., buildValuesForFileFromCache or getValuesForFileOnly) that returns
only valuesForFile based on this.cache_ and this.keyStorage/key checks, and a
separate method (e.g., getDbUpdateQueriesWithKeychainWrites or
applyKeychainAndBuildQueries) that performs the keychain writes and builds the
queries; then update saveAll to call both the pure getter and the side-effecting
method to get queries, while making resetDefaultProfileSettings call only the
pure getter and then use splitGlobalAndLocalSettings as before so no keychain
writes or discarded DB queries occur during reset.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 3cdd61b8-5737-4ee2-812c-d10376a7bf3e
📒 Files selected for processing (3)
packages/app-desktop/gui/ProfileEditor.tsxpackages/lib/models/Setting.tspackages/lib/models/settings/FileHandler.ts
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)
packages/lib/models/settings/FileHandler.ts (1)
56-85:⚠️ Potential issue | 🟡 Minor
parsedJsonCache_can become stale relative tovalueJsonCache_on the non-overwrite path.After a non-overwrite
save(),valueJsonCache_is refreshed (line 79) butparsedJsonCache_is left untouched. Ifvaluesintroduces any key not already present inparsedJsonCache_, a subsequentload()will short-circuit at line 34 and return the stale cached object, which is missing the newly saved keys (and any value updates for keys already in the cache, since the merge loop only fills in keys absent fromvalues). Prior behaviour unconditionally replaced the cache, so this is a behavioural regression for the default path.Consider updating
parsedJsonCache_on both paths (it already reflects the mergedvaluesafter the loop):🛠️ Proposed fix
await shim.fsDriver().writeFile(this.filePath_, json, 'utf8'); this.valueJsonCache_ = json; - - if (overwrite) { - // Prevent pre-existing settings from being re-instated by subsequent saving of settings - this.parsedJsonCache_ = values; - } + // Keep parsedJsonCache_ in sync with what was just written. On the overwrite path this + // also prevents pre-existing settings from being re-instated by subsequent saves. + this.parsedJsonCache_ = values;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/lib/models/settings/FileHandler.ts` around lines 56 - 85, The save() method updates valueJsonCache_ but on the non-overwrite path it never updates parsedJsonCache_, causing the parsed cache to remain stale; after the merge loop and successful write (i.e. after await shim.fsDriver().writeFile(...) and setting this.valueJsonCache_), assign the merged values into this.parsedJsonCache_ (this.parsedJsonCache_ = values) so parsedJsonCache_ reflects the newly saved/merged settings; keep the existing overwrite-specific assignment but ensure parsedJsonCache_ is updated on the general save path as well.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@packages/lib/models/settings/FileHandler.ts`:
- Around line 56-85: The save() method updates valueJsonCache_ but on the
non-overwrite path it never updates parsedJsonCache_, causing the parsed cache
to remain stale; after the merge loop and successful write (i.e. after await
shim.fsDriver().writeFile(...) and setting this.valueJsonCache_), assign the
merged values into this.parsedJsonCache_ (this.parsedJsonCache_ = values) so
parsedJsonCache_ reflects the newly saved/merged settings; keep the existing
overwrite-specific assignment but ensure parsedJsonCache_ is updated on the
general save path as well.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: e756583f-1239-4ee9-a96c-04809df50f7e
📒 Files selected for processing (1)
packages/lib/models/settings/FileHandler.ts
2 participants
On both desktop and mobile, it is not possible to delete the default profile from the UI. This is because the default profile needs to be handled differently, because data from other profiles sit within the directory of the default profile, and some global settings are also stored there.
This PR implements deletion of the default profile, by 'resetting' the default profile, rather than removing the profile from the profile list. On desktop, this is done by explicitly removing the default profile specific directories and files by name, and replacing the settings.json with json just containing the global settings. On mobile, the same is done, except resources need special handling, but there is no settings file needed to be deleted / cleaned. As resources on the default profile exist at the root of the app directory, they are no isolated from global preferences and data from other profiles. Therefore, resources for deletion are detected by listing the files in the directory, and deleting any files which have a filename which is an alphanumeric string of exactly 32 characters without the extension. No shared files or files relating to other profiles meet this criteria, which make it a safe way to match the relevant items. Temporary encrypted resources also match this criteria, and will therefore be deleted as well.
After deleting the default profile, a popup is shown which says 'The default profile has been reset', in order to provide feedback to the user the action has completed, because the default profile remains in the list.
This fixes #14506
Testing
Scenarios tested:
Video on desktop:
https://github.com/user-attachments/assets/f5661990-c22f-4faa-8f1a-432fd53a75cb
Video on mobile:
https://github.com/user-attachments/assets/ab6e19d5-1015-4634-80d7-72f249e64924