Add per-device KeeShare sync mode#13089
Open
Lcstyle wants to merge 5 commits intokeepassxreboot:developfrom
Open
Add per-device KeeShare sync mode#13089Lcstyle wants to merge 5 commits intokeepassxreboot:developfrom
Lcstyle wants to merge 5 commits intokeepassxreboot:developfrom
Conversation
Enable each KeePassXC instance to write its own container file
({DEVICE_ID}.kdbx) to a shared sync directory while importing from
all other devices' files. This eliminates Syncthing file conflicts
when multiple devices share a group.
When a KeeShare reference path points to a directory instead of a
.kdbx file, per-device mode activates automatically:
- Export writes to {syncDir}/{DEVICE_ID}.kdbx
- Import reads all .kdbx files in the directory except own device
- Classic single-file mode is fully preserved
Changes:
- Add KeeShare_DeviceId config key with auto-detection fallback
- Add Reference::isPerDeviceMode() detection based on path extension
- Extend ShareObserver with QFileSystemWatcher for directory watching
- Add importPerDeviceShares() for multi-file import from sync dirs
- Update EditGroupWidgetKeeShare with directory selection dialog
- Add Device Identity settings field to SettingsWidgetKeeShare
- Add KeeShare/PerDeviceSync custom data support for KeePassDX interop
- Add unit tests for per-device mode path detection
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Register QMap<QString,QString> as a D-Bus metatype so Polkit CheckAuthorization can marshal the details argument. Without this, Quick Unlock fails with "Unregistered type QMap<QString,QString> passed in arguments".
Contributor
There was a problem hiding this comment.
Pull request overview
This pull request adds a per-device sync mode for KeeShare that eliminates file conflicts when syncing via tools like Syncthing or Nextcloud. The key innovation is allowing each device to write to its own container file ({DEVICE_ID}.kdbx) within a shared directory, instead of all devices writing to the same file. The PR also includes an independent fix for a Quick Unlock Polkit D-Bus marshalling bug.
Changes:
- Per-device sync mode: When a KeeShare path points to a directory (vs. a
.kdbxfile), each device exports to{DEVICE_ID}.kdbxin that directory and imports from all other*.kdbxfiles found there - Device ID management: Auto-generated from system machine ID or manually configurable via settings UI
- Quick Unlock D-Bus fix: Registers
QMap<QString, QString>as D-Bus metatype to fix Polkit authorization marshalling errors
Reviewed changes
Copilot reviewed 15 out of 15 changed files in this pull request and generated 19 comments.
Show a summary per file
| File | Description |
|---|---|
| src/quickunlock/Polkit.cpp | Adds D-Bus metatype registration for QMap<QString, QString> to fix Polkit marshalling |
| src/core/Config.h/cpp | Adds KeeShare_DeviceId config key for storing device identifier |
| src/keeshare/KeeShare.h/cpp | Implements deviceId() getter/setter with auto-generation and sanitization logic |
| src/keeshare/KeeShareSettings.h/cpp | Adds isPerDeviceMode() method to detect directory-based sync paths |
| src/keeshare/ShareObserver.h/cpp | Implements per-device import/export logic with QFileSystemWatcher for directories |
| src/keeshare/SettingsWidgetKeeShare.ui/cpp | Adds device ID configuration UI field |
| src/keeshare/group/EditGroupWidgetKeeShare.ui/cpp | Updates path selection dialog and validation for directory mode |
| tests/TestSharing.h/cpp | Adds unit tests for per-device mode path detection |
Comments suppressed due to low confidence (1)
src/keeshare/group/EditGroupWidgetKeeShare.cpp:158
- The conflict detection logic doesn't account for per-device mode. When multiple groups export to the same directory in per-device mode, they each create separate device files ({DEVICE_ID}.kdbx), so there's no actual conflict. However, the current code would flag this as a "conflictExport" error (line 151). The conflict detection should either skip per-device mode paths or compare the full resolved export path (directory + device file) instead of just the directory path.
const auto groups = m_database->rootGroup()->groupsRecursive(true);
bool conflictExport = false;
bool multipleImport = false;
bool cycleImportExport = false;
for (const auto* group : groups) {
if (group->uuid() == m_temporaryGroup->uuid()) {
continue;
}
const auto other = KeeShare::referenceOf(group);
if (other.path != reference.path) {
continue;
}
multipleImport |= other.isImporting() && reference.isImporting();
conflictExport |= other.isExporting() && reference.isExporting();
cycleImportExport |=
(other.isImporting() && reference.isExporting()) || (other.isExporting() && reference.isImporting());
}
if (conflictExport) {
m_ui->messageWidget->showMessage(tr("%1 is already being exported by this database.").arg(reference.path),
MessageWidget::Error);
return;
Comment on lines
+252
to
+266
| // For SynchronizeWith, offer both file and directory selection | ||
| if (reference.type == KeeShareSettings::SynchronizeWith) { | ||
| // Try directory selection first for per-device sync | ||
| auto dirPath = fileDialog()->getExistingDirectory( | ||
| this, tr("Select per-device sync directory"), defaultDirPath); | ||
| if (!dirPath.isEmpty()) { | ||
| // Directory selected: per-device mode | ||
| m_ui->pathEdit->setText(dirPath); | ||
| selectPath(); | ||
| FileDialog::saveLastDir("keeshare", dirPath); | ||
| updateSharingState(); | ||
| return; | ||
| } | ||
| // User cancelled directory dialog; fall through to file dialog | ||
| } |
There was a problem hiding this comment.
The dialog flow is confusing: when SynchronizeWith is selected, the code shows a directory picker first, and if the user cancels, it falls through to show a file picker. This means canceling the directory picker doesn't actually cancel the operation - it forces the user into file selection mode. This is unintuitive UX. Consider either offering a choice upfront, or respecting the cancellation.
Comment on lines
+405
to
+419
| if (reference.config.isPerDeviceMode()) { | ||
| // Per-device mode: export to {directory}/{DEVICE_ID}.kdbx | ||
| QDir dir(resolvedPath); | ||
| if (!dir.exists()) { | ||
| dir.mkpath("."); | ||
| } | ||
| const auto deviceFile = dir.absoluteFilePath(KeeShare::deviceId() + ".kdbx"); | ||
|
|
||
| // Pause directory watcher during export | ||
| auto dirWatcher = m_dirWatchers.value(resolvedPath); | ||
| if (dirWatcher) { | ||
| dirWatcher->removePath(resolvedPath); | ||
| } | ||
|
|
||
| results << ShareExport::intoContainer(deviceFile, reference.config, reference.group); |
There was a problem hiding this comment.
The conflict detection for exports doesn't account for per-device mode. When multiple groups export to the same directory in per-device mode, they each create separate device files ({DEVICE_ID}.kdbx) within that directory, so there's no actual conflict. However, the validation code before this region (lines 385-398, not shown in diff) treats this as a "Conflicting export target path" error. The per-device mode export logic should work with the conflict detection to allow multiple groups to export to the same directory when using per-device mode.
Author
|
Damn! your copilot review is awesome! Caught a bunch of stuff! Fantastic man I love that, all automated too! @droidmonkey |
…emove dead code - split m_inFileUpdate into separate file/dir flags to prevent dropped updates - check mkpath return value and report error on failure - add QRegularExpressionValidator and length cap (32) to device ID - clear device ID config when field emptied (allows auto-detect reset) - remove unused hasPerDeviceConfig/perDeviceSyncPath and KeeShare_PerDeviceSync - skip cycleImportExport warning in per-device mode (not a real conflict) - move tooltip from pathLabel to pathEdit input field - add testPerDeviceModeImportExport test coverage
Author
Copilot Review: All 19 Comments VerifiedFixed (8 items):
Pushback — Copilot is wrong or overstating (6 items):
Acknowledged but not changing (5 items):
|
Author
|
@droidmonkey hi there! |
droidmonkey
requested changes
Mar 1, 2026
Comment on lines
+289
to
+294
| bool Reference::isPerDeviceMode() const | ||
| { | ||
| return !path.isEmpty() | ||
| && !path.endsWith(".kdbx", Qt::CaseInsensitive) | ||
| && !path.endsWith(".kdbx.share", Qt::CaseInsensitive); | ||
| } |
Member
There was a problem hiding this comment.
I agree, this check is way too loose. We know what directories are per-device mode enabled already, test against what we know.
Member
|
I did a quick run through, copilot made some great recommendations so read through the remaining comments and make adjustments. |
Move all sanitization (strip non-alphanumeric, truncate to 32, fallback to DEFAULT) into setDeviceId() so it's consistent regardless of call site. deviceId() now delegates to setDeviceId() and re-reads the sanitized value. isPerDeviceMode() now uses QFileInfo::isDir() as a positive filesystem check — no extension-based heuristics. Tests updated to create real temp directories instead of using synthetic paths.
Change isPerDeviceMode(const QString&) to isPerDeviceMode(const QDir& baseDir) so the method resolves this->path against the caller-provided base directory. This fixes silent misclassification when reference paths are relative, since QFileInfo previously resolved against process CWD instead of the database directory. Override Result.path with the actual device file path in importPerDeviceShares so status messages show which specific container failed, not just the sync directory. Add regression tests for relative directory and file paths.
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
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
{DEVICE_ID}.kdbx) into a shared directory, eliminating file conflicts when syncing via Syncthing/Nextcloud/etc..kdbxor.kdbx.share, it works exactly as beforeHow per-device mode works
When a KeeShare reference path points to a directory (instead of a
.kdbxfile), KeePassXC treats it as per-device mode:{syncDir}/{DEVICE_ID}.kdbx*.kdbxfiles in the directory except the device's own fileThe existing
ShareObserverseparation (import on directory change, export on database save) naturally prevents feedback loops between devices.Commits
QMap<QString, QString>as D-Bus metatype — fixes "Unregistered type" error on PolkitCheckAuthorization. Independent fix, cherry-pickable separatelyInterop
Matching KeePassDX (Android) implementation: Kunzisoft/KeePassDX#2438
Discussion: #13080
Tested with real Syncthing sync between Fedora desktop and Pixel 7 running KeePassDX.
Test plan
.kdbx)