Add configuration synchronization with S3 and WebDAV support#96
Open
mrhuangyong wants to merge 22 commits into
Open
Add configuration synchronization with S3 and WebDAV support#96mrhuangyong wants to merge 22 commits into
mrhuangyong wants to merge 22 commits into
Conversation
Design for cross-device configuration synchronization supporting S3 and WebDAV backends with end-to-end encryption (PBKDF2 + AES-256-GCM), hybrid auto/manual sync mode, and Last-Write-Wins conflict resolution. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
10-task plan covering Rust backend (crypto, S3, WebDAV providers, SyncManager, Tauri commands) and React frontend (sync settings panel, API layer). Follows TDD with bite-sized commits. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Add sha2, hmac, pbkdf2, hex dependencies for sync crypto. Create sync_state table migration for storing sync configuration, device ID, and sync metadata. Add get/set/delete CRUD methods. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Define SyncProvider trait with test_connection/put/get/delete. Add SyncConfig, SyncStatus, SyncResult, SyncSnapshot types. Include build_provider factory and stub S3/WebDAV implementations. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
PBKDF2-SHA256 key derivation (600k iterations) with AES-256-GCM encryption for sync snapshots. Includes snapshot_hash for change detection and encrypt_with_key/decrypt_with_key for local credential storage. All 5 unit tests passing. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Implements SyncProvider for S3-compatible storage using reqwest with manual AWS Signature V4 signing. Supports any S3-compatible endpoint (AWS, MinIO, OSS, etc.). Added url crate dependency. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Implements SyncProvider for WebDAV servers using reqwest with basic auth. Supports PROPFIND for connection testing and standard HTTP PUT/GET/DELETE for object operations. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Core sync orchestration: export_snapshot serializes local data (connections, queries, AI providers) to encrypted snapshots. import_snapshot clears local tables and re-imports from remote. Supports configure, sync_now, force_push, force_pull, and auto_sync_push with change detection via SHA-256 hash comparison. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Add 8 sync commands: test_connection, configure, get_status, sync_now, force_push, force_pull, disable, update_password. Wrap local_db in Arc<Mutex> in AppState for SyncManager access. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Add SyncConfig, SyncStatus, SyncResult types and syncApi namespace with testConnection, configure, getStatus, syncNow, forcePush, forcePull, disable, and updatePassword methods. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Add SyncSettings panel with provider configuration (S3/WebDAV), sync password setup, test connection, and sync status display. Integrate as new "Sync" tab in SettingsDialog with Cloud icon. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
The rename_all="camelCase" converted WebDAV to "webDAV" which didn't match the frontend's "WebDAV". Use explicit #[serde(rename)] to ensure exact string match: "S3" and "WebDAV". Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Add proper internationalization keys for all sync UI text in en/zh/ja locales, replacing defaultValue fallbacks. Add sync_get_config backend command to retrieve saved config so form fields are populated when reopening the Sync settings tab. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…ter it Encrypt the sync password using the existing ai_master_key and store it in sync_state table. Sync Now, Force Push, and Force Pull operations now automatically retrieve the stored password instead of requiring manual input each time. Password fields are hidden from the UI when sync is already enabled. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Add passwordStored field to SyncStatus so the frontend can detect when sync was configured before the password persistence feature. In that case, show a password input with a hint to re-enter the sync password once, which gets stored encrypted for future automatic use. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
The sync snapshot stores Connection objects which use the field name "dbType", but create_connection expects ConnectionForm with "driver". Add a JSON field mapping during import to fix the deserialization error. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Implement automatic sync using two complementary mechanisms: A) Periodic timer: SyncScheduler runs every 5 minutes, checks if sync is enabled and local data has changed, then auto-pushes to remote. B) Event-driven push: After any data mutation (create/update/delete connection, saved query, or AI provider), notify_data_changed() is called which triggers a debounced (3s delay) auto-sync push. The scheduler starts on app launch and stops on exit. It only acts when sync is enabled and the password has been stored. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Add syncIntervalMinutes to SyncConfig (default: 5 min). Users can choose from 1/3/5/10/30/60 minute intervals in the sync settings UI. The scheduler reads the configured interval from the database on each cycle, so changes take effect without restarting the app. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…ctor The scheduler was calling tokio::spawn directly, which panics outside a Tokio runtime context. Switch to tauri::async_runtime::spawn which properly uses Tauri's managed runtime. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
feat: add config sync with S3/WebDAV support
📝 WalkthroughWalkthroughThis PR implements comprehensive cross-device configuration synchronization for DbPaw, allowing users to synchronize database connections, saved queries, AI provider settings, and user preferences across multiple devices via S3 or WebDAV backends with end-to-end AES-256-GCM encryption, automatic periodic syncing, and manual sync controls. ChangesConfig Sync Feature
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
Review Summary by QodoAdd configuration synchronization with S3 and WebDAV support
WalkthroughsDescription• Implements comprehensive configuration synchronization system with S3 and WebDAV support • Adds end-to-end encryption using AES-256-GCM with PBKDF2-SHA256 key derivation for secure data protection • Core SyncManager handles snapshot export/import with connections, saved queries, and AI providers • Implements SyncProvider trait with S3 provider using AWS Signature V4 and WebDAV provider with HTTP basic auth • Adds SyncScheduler for periodic auto-sync with configurable intervals and debounced push on local data changes • Integrates sync notifications across connection, storage, and AI provider commands • Extends AppState with SyncScheduler and manages lifecycle through app initialization and exit • Adds database migration for sync_state table to persist sync configuration and state • Provides Tauri command handlers for sync operations: test connection, configure, get status, manual sync, force push/pull, disable, and password updates • Implements React UI component SyncSettings for sync configuration with provider selection and status display • Adds TypeScript API types and service methods for frontend-backend communication • Includes comprehensive translations in English, Japanese, and Chinese • Adds required cryptographic dependencies: sha2, hmac, pbkdf2, hex, and url Diagramflowchart LR
A["Local Data<br/>Connections, Queries, AI"] -->|"notify_data_changed"| B["SyncScheduler"]
B -->|"periodic/debounced"| C["SyncManager"]
C -->|"snapshot export/import"| D["Encryption<br/>AES-256-GCM"]
D -->|"encrypted snapshot"| E["SyncProvider"]
E -->|"S3 or WebDAV"| F["Remote Storage"]
G["Tauri Commands"] -->|"configure, sync_now,<br/>force_push/pull"| C
H["React UI<br/>SyncSettings"] -->|"user actions"| G
C -->|"persist state"| I["sync_state Table"]
File Changes1. src-tauri/src/sync/manager.rs
|
Code Review by Qodo
1. api.sync missing mocks
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 813b9faf5c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| method, path, query, canonical_headers, signed_headers, payload_hash | ||
| ); | ||
|
|
||
| let credential_scope = format!("{}/{}/s3/aws4_request", self.region, date); |
There was a problem hiding this comment.
For S3 sync, every signed request uses this credential scope, but AWS Signature V4 expects date/region/service/aws4_request, not region/date/service/aws4_request. With real S3-compatible services that verify SigV4, the Authorization header will not match and testConnection, put_object, and get_object all fail for otherwise valid S3 settings.
Useful? React with 👍 / 👎.
Comment on lines
+485
to
+488
| let connection_id = q_val | ||
| .get("connectionId") | ||
| .or_else(|| q_val.get("connection_id")) | ||
| .and_then(|v| v.as_i64()); |
There was a problem hiding this comment.
When a snapshot is imported on another device, connections are recreated with fresh autoincrement ids because create_connection ignores the snapshot's id/uuid, but saved queries are then reinserted with the original numeric connectionId. If the destination ids differ, imported saved queries point at the wrong connection or no connection at all; the import needs to map snapshot connection identifiers to the newly created ids before creating queries.
Useful? React with 👍 / 👎.
Comment on lines
+91
to
+95
| // Save config | ||
| let config_json = serde_json::to_string(config) | ||
| .map_err(|e| format!("[SYNC_CONFIG_ERROR] Serialize config: {e}"))?; | ||
|
|
||
| db.set_sync_state("sync_config", &config_json).await?; |
There was a problem hiding this comment.
When configuring S3 or WebDAV, SyncConfig contains secretAccessKey/WebDAV password, and serializing the whole struct here writes those secrets directly into sync_state and later returns them through sync_get_config. This bypasses the local encryption used for other sync secrets, so anyone with the local database can read credentials that grant access to the remote sync store.
Useful? React with 👍 / 👎.
Comment on lines
+96
to
+99
| // Check if local data has changed | ||
| if !manager.has_local_changes().await? { | ||
| return Ok(()); | ||
| } |
There was a problem hiding this comment.
The periodic scheduler exits whenever there are no local changes, so an idle device never downloads snapshots pushed by another device unless the user manually clicks sync or first makes a local edit. That breaks automatic cross-device synchronization in the common case where one machine changes settings and another is just waiting for updates; the scheduled path should call a pull-capable sync flow instead of gating entirely on local changes.
Useful? React with 👍 / 👎.
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
Comment on lines
+1776
to
+1789
| sync: { | ||
| testConnection: (config: SyncConfig): Promise<void> => | ||
| invoke("sync_test_connection", { config }), | ||
| configure: (config: SyncConfig, syncPassword: string): Promise<void> => | ||
| invoke("sync_configure", { config, syncPassword }), | ||
| getStatus: (): Promise<SyncStatus> => invoke("sync_get_status"), | ||
| getConfig: (): Promise<SyncConfig | null> => invoke("sync_get_config"), | ||
| syncNow: (): Promise<SyncResult> => invoke("sync_now"), | ||
| forcePush: (): Promise<void> => invoke("sync_force_push"), | ||
| forcePull: (): Promise<void> => invoke("sync_force_pull"), | ||
| disable: (): Promise<void> => invoke("sync_disable"), | ||
| updatePassword: (oldPassword: string, newPassword: string): Promise<void> => | ||
| invoke("sync_update_password", { oldPassword, newPassword }), | ||
| }, |
There was a problem hiding this comment.
1. api.sync missing mocks 📘 Rule violation ☼ Reliability
New api.sync methods were added to src/services/api.ts but there are no corresponding sync_* handlers in src/services/mocks.ts, so Mock mode will throw Mock: Unknown command for these calls. This breaks the compliance requirement that every API method must be mockable when VITE_USE_MOCK=true.
Agent Prompt
## Issue description
`src/services/api.ts` introduces new `api.sync.*` methods that call Tauri commands (`sync_test_connection`, `sync_configure`, `sync_get_status`, etc.). In Mock mode (`VITE_USE_MOCK=true`), these commands route through `invokeMock()` in `src/services/mocks.ts`, but there are no `case "sync_*"` handlers, so the default branch throws `Mock: Unknown command`.
## Issue Context
Compliance requires that every new API method added to `src/services/api.ts` has a corresponding mock implementation in `src/services/mocks.ts` to keep Mock mode complete.
## Fix Focus Areas
- src/services/mocks.ts[1836-2437]
- src/services/api.ts[1776-1789]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+91
to
+97
| let credential_scope = format!("{}/{}/s3/aws4_request", self.region, date); | ||
| let string_to_sign = format!( | ||
| "AWS4-HMAC-SHA256\n{}\n{}\n{}", | ||
| datetime, | ||
| credential_scope, | ||
| hex::encode(sha256(canonical_request.as_bytes())) | ||
| ); |
There was a problem hiding this comment.
2. Sigv4 scope order wrong 🐞 Bug ≡ Correctness
S3Provider::sign_request builds credential_scope as {region}/{date}/s3/aws4_request instead of
{date}/{region}/s3/aws4_request, producing an invalid string-to-sign and signature. All S3 sync
operations will fail with signature mismatch errors.
Agent Prompt
### Issue description
S3 SigV4 signing uses the wrong credential scope component ordering (`region/date`), which makes signatures invalid and breaks all S3 operations.
### Issue Context
In SigV4, credential scope must be `YYYYMMDD/region/service/aws4_request`.
### Fix Focus Areas
- src-tauri/src/sync/s3.rs[91-107]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+187
to
+198
| // Import remote data if it's from a different device | ||
| if remote_snapshot.device_id != local_device_id { | ||
| self.import_snapshot(&db, &remote_snapshot).await?; | ||
| db.set_sync_state("last_sync_result", "success").await?; | ||
| db.set_sync_state("last_sync_at", &now).await?; | ||
|
|
||
| return Ok(SyncResult { | ||
| action: "pulled".to_string(), | ||
| timestamp: now, | ||
| remote_device_id: Some(remote_snapshot.device_id), | ||
| }); | ||
| } |
There was a problem hiding this comment.
3. Pull sync state not saved 🐞 Bug ≡ Correctness
SyncManager::sync_now imports a remote snapshot from another device but does not update last_synced_hash, so subsequent runs still consider the local DB “changed” and will keep re-syncing unnecessarily. This also prevents the app from recognizing it is already in-sync after a pull.
Agent Prompt
### Issue description
After a successful pull/import in `sync_now`, the code does not persist `last_synced_hash`, so change detection stays stale.
### Issue Context
`has_local_changes()` compares the exported snapshot hash to `last_synced_hash`; if the pulled branch never writes it, auto-sync and future sync decisions are incorrect.
### Fix Focus Areas
- src-tauri/src/sync/manager.rs[169-217]
- src-tauri/src/sync/manager.rs[313-324]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+444
to
+495
| // Clear and re-import connections | ||
| let existing_connections = db.list_connections().await?; | ||
| for conn in &existing_connections { | ||
| db.delete_connection(conn.id).await?; | ||
| } | ||
| for conn_val in &snapshot.data.connections { | ||
| // Snapshot stores Connection objects (dbType), but create_connection | ||
| // expects ConnectionForm (driver). Map the field name. | ||
| let mut form_val = conn_val.clone(); | ||
| if let Some(db_type) = form_val.get("dbType").cloned() { | ||
| form_val.as_object_mut().map(|m| { | ||
| m.insert("driver".to_string(), db_type); | ||
| m | ||
| }); | ||
| } | ||
| let form: crate::models::ConnectionForm = | ||
| serde_json::from_value(form_val) | ||
| .map_err(|e| format!("[SYNC_MERGE_ERROR] Parse connection: {e}"))?; | ||
| db.create_connection(form).await?; | ||
| } | ||
|
|
||
| // Clear and re-import saved queries | ||
| let existing_queries = db.list_saved_queries().await?; | ||
| for q in &existing_queries { | ||
| db.delete_saved_query(q.id).await?; | ||
| } | ||
| for q_val in &snapshot.data.saved_queries { | ||
| let name = q_val | ||
| .get("name") | ||
| .and_then(|v| v.as_str()) | ||
| .unwrap_or("") | ||
| .to_string(); | ||
| let query = q_val | ||
| .get("query") | ||
| .and_then(|v| v.as_str()) | ||
| .unwrap_or("") | ||
| .to_string(); | ||
| let description = q_val | ||
| .get("description") | ||
| .and_then(|v| v.as_str()) | ||
| .map(String::from); | ||
| let connection_id = q_val | ||
| .get("connectionId") | ||
| .or_else(|| q_val.get("connection_id")) | ||
| .and_then(|v| v.as_i64()); | ||
| let database = q_val | ||
| .get("database") | ||
| .and_then(|v| v.as_str()) | ||
| .map(String::from); | ||
| db.create_saved_query(name, query, description, connection_id, database) | ||
| .await?; | ||
| } |
There was a problem hiding this comment.
4. Saved query links broken 🐞 Bug ≡ Correctness
import_snapshot recreates connections with new autoincrement IDs but imports saved queries using the snapshot’s connectionId, leaving saved queries pointing at non-existent connections. This breaks saved-query to connection association after any pull/force-pull.
Agent Prompt
### Issue description
During snapshot import, connections are deleted and recreated, which changes their DB IDs; saved queries are then imported with stale `connectionId` values.
### Issue Context
`LocalDb::create_connection` always inserts a new row and returns a new ID. Snapshot entries include the old IDs; without an old->new mapping, `saved_queries.connection_id` becomes orphaned.
### Fix Focus Areas
- src-tauri/src/sync/manager.rs[444-495]
- src-tauri/src/db/local.rs[390-451]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+91
to
+101
| // Save config | ||
| let config_json = serde_json::to_string(config) | ||
| .map_err(|e| format!("[SYNC_CONFIG_ERROR] Serialize config: {e}"))?; | ||
|
|
||
| db.set_sync_state("sync_config", &config_json).await?; | ||
| db.set_sync_state( | ||
| "provider_type", | ||
| &format!("{:?}", config.provider_type), | ||
| ) | ||
| .await?; | ||
|
|
There was a problem hiding this comment.
5. Credentials stored plaintext 🐞 Bug ⛨ Security
SyncManager::configure stores the full serialized SyncConfig into sync_state, which includes S3 secretAccessKey and WebDAV password in cleartext. Compromise of the local SQLite DB exposes remote storage credentials.
Agent Prompt
### Issue description
Provider credentials (S3 secret access key, WebDAV password) are persisted unencrypted in `sync_state.sync_config`.
### Issue Context
`SyncConfig` contains sensitive fields (`secret_access_key`, `password`) and is saved verbatim as JSON.
### Fix Focus Areas
- src-tauri/src/sync/manager.rs[91-117]
- src-tauri/src/sync/provider.rs[14-29]
- src-tauri/src/db/local.rs[317-326]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 17
🧹 Nitpick comments (2)
src/components/settings/SettingsDialog.tsx (1)
58-58: ⚡ Quick winUse the
@/alias for the new settings import.Please keep this import consistent with the repo-wide TS import convention.
Suggested change
-import { SyncSettings } from "./SyncSettings"; +import { SyncSettings } from "`@/components/settings/SyncSettings`";As per coding guidelines,
src/**/*.{ts,tsx}: Use path alias@/mapping to./src/for imports in TypeScript files.🤖 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 `@src/components/settings/SettingsDialog.tsx` at line 58, Update the import in SettingsDialog.tsx to use the repo TypeScript path alias instead of a relative path; replace the current import of SyncSettings (import { SyncSettings } from "./SyncSettings";) with the aliased module import (use "`@/`..." rooted at src) so the component imports follow the repo-wide convention.src-tauri/src/sync/scheduler.rs (1)
15-112: 🏗️ Heavy liftAdd unit coverage for the scheduler contract.
The interval clamp, shutdown path, and change-trigger behavior are the core logic in this new module, but there are no source-level tests locking them down. As per coding guidelines,
src-tauri/**/*.rs: Use Rust unit tests marked with#[test]in source files, run withcargo test --lib, and use#[ignore]for integration tests that should only run explicitly with specific database environment variables.🤖 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 `@src-tauri/src/sync/scheduler.rs` around lines 15 - 112, Add source-level unit tests to src-tauri/src/sync/scheduler.rs to cover the interval clamping, shutdown path, and change-trigger behavior: create a #[cfg(test)] mod tests in the same file and add tests that exercise SyncScheduler::get_sync_interval_secs (verify clamping and default), SyncScheduler::start/stop (spawn the scheduler, send stop via stop() and assert it exits/shuts down), and notify_data_changed (ensure try_auto_sync is invoked after debounce) using tokio async tests (#[tokio::test]) so the async runtime and tokio::time can be controlled (use tokio::time::pause/advance for timing); mark only long/integration-like tests with #[ignore] per guidelines and keep tests deterministic by injecting or mocking the LocalDb/SyncManager behaviors referenced in try_auto_sync / manager methods.
🤖 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/superpowers/plans/2026-06-02-config-sync.md`:
- Around line 1065-1070: The code currently serializes the entire SyncConfig
(via serde_json::to_string into config_json) and persists it with
db.set_sync_state("sync_config", &config_json), which will persist provider
secrets in plaintext; instead, filter out or redact sensitive fields (e.g. S3
secret_key, WebDAV password) before serializing or replace them with non-secret
placeholders, and persist actual credentials separately using the intended
encrypted/secure storage mechanism (or call the existing credential store API)
so that db.set_sync_state("sync_config", ...) only receives a non-sensitive
representation of SyncConfig.
- Line 1320: export_snapshot() currently hardcodes settings to an empty object
and import_snapshot() skips applying settings/shortcuts, so user settings and
keyboard shortcuts never get synced; update export_snapshot() to read and
include the real settings/shortcuts (e.g., load from settings.json or the
settings store used elsewhere) instead of
serde_json::Value::Object(serde_json::Map::new()), and update import_snapshot()
to apply those settings/shortcuts during import (use the same application
logic/handlers that the app uses to persist settings and shortcuts), ensuring
the snapshot payload contains the settings and import_snapshot() processes the
settings and shortcuts entries rather than skipping them.
- Around line 1131-1142: The current code imports any snapshot from another
device without checking its timestamp; before calling import_snapshot(&db,
&remote_snapshot) (inside the branch that compares remote_snapshot.device_id !=
local_device_id) fetch the local snapshot's timestamp (e.g., via the existing
local snapshot lookup or a db method that returns the latest snapshot timestamp)
and compare timestamps per Last-Write-Wins: only proceed to import if
remote_snapshot.timestamp is strictly newer than the local timestamp; if it's
older or equal, skip import, set appropriate sync state (e.g.,
"last_sync_result" => "ignored" or "no-op") and return a SyncResult indicating
no pull occurred instead of overwriting newer local data.
In `@docs/superpowers/specs/2026-06-02-config-sync-design.md`:
- Around line 137-148: Remove the fast password verifier stored as the key
"sync_password_hash" in the sync_state table and any code paths that read/write
that key (references to sync_state and "sync_password_hash"); either drop the
key from the schema and associated logic, or if a verifier must be kept, replace
it with a salted, slow KDF output (e.g., PBKDF2/Argon2) and update all
read/write/verification code to use the KDF output and salt rather than a raw
hash so offline brute-force is not trivial.
In `@src-tauri/src/commands/sync.rs`:
- Around line 13-19: Validate SyncConfig.sync_interval_minutes in sync_configure
and reject values equal to 0 before calling SyncManager::configure: check
config.sync_interval_minutes == 0 and return an Err with a clear message (e.g.
"sync_interval_minutes must be >= 1") so the invalid value is not persisted;
reference sync_configure, SyncConfig.sync_interval_minutes,
SyncManager::configure, SyncScheduler::get_sync_interval_secs, and
sync_get_status when making the change.
In `@src-tauri/src/sync/manager.rs`:
- Around line 390-423: export_snapshot currently builds the snapshot from
LocalDb::list_connections(), which returns redacted connection records
(sentinel_password, api_key_secret, api_key_encoded nulled) so exported
snapshots miss required secrets; change export_snapshot to fetch connections
with their real secret fields (e.g., call a new or existing non-redacting method
such as LocalDb::list_connections_unredacted or
LocalDb::list_connections_with_secrets instead of LocalDb::list_connections()),
then keep the existing AI provider decrypt/re-encrypt logic
(db.decrypt_ai_api_key / LocalDb::has_encrypted_ai_api_key) but operate on the
unredacted connection structs so exported snapshots include the full
Redis/Elasticsearch/cloud config fields.
- Around line 445-495: When recreating connections, capture each snapshot
connection’s original ID (e.g., "id" or "connection_id") before you call
db.create_connection and store a mapping from original_id -> newly_created_id
(the value returned by db.create_connection); then, when importing saved queries
in the saved_queries loop, look up the snapshot query’s
connectionId/connection_id in that mapping and pass the mapped new local
connection id to db.create_saved_query (or None if no mapping found). Implement
this with a HashMap collected in the connections-create loop (update the block
that clones conn_val and calls db.create_connection) and consult that map in the
saved_queries import block to translate connection references.
- Around line 422-423: The snapshot currently hardcodes settings to an empty
object (settings: serde_json::Value::Object(...)) and never applies imported
settings; update the snapshot creation in the export path (e.g., where the
Snapshot struct or export_snapshot is constructed) to serialize and include the
live application settings instead of an empty map, and update the import path
(e.g., import_snapshot/apply_snapshot or wherever snapshots are deserialized) to
read the snapshot.settings Value and merge/apply it into the application's
settings store (call the existing SettingsManager/Settings API such as
set_settings/apply_from_json or deserialize into the Settings type) so user
preferences are persisted on import. Ensure appropriate error handling when
deserializing/merging the serde_json::Value.
- Around line 169-199: The current pull path unconditionally calls
import_snapshot(&db, &remote_snapshot) and returns, which can overwrite local
unsynced edits; before calling import_snapshot (in the branch where
remote_snapshot.device_id != local_device_id) detect whether the local device
has pending/unsynced changes (e.g., via an existing DB method or by computing
local snapshot/data and comparing hashes), and if unsynced changes exist avoid
the destructive import: either return a conflict result (e.g., an Err or a
SyncResult indicating "conflict") or trigger a merge flow instead of
import_snapshot; ensure you still update sync state only after a non-destructive
successful resolution and preserve local connections/queries/AI providers when
reporting the conflict.
- Around line 444-510: Validate and convert the entire snapshot into DB-ready
objects first (parse all connection entries into crate::models::ConnectionForm,
saved queries into the needed fields, and AiProviderForm) and return an error on
any parse failure before performing any deletes, then wrap the
clear-and-reimport sequence (the loops that call db.delete_connection /
db.create_connection, db.delete_saved_query / db.create_saved_query, and
db.delete_ai_provider / db.create_ai_provider) inside a single database
transaction (use your DB client's transaction API, e.g., db.transaction() or
similar) so that you perform deletes and inserts atomically and rollback on
error; ensure you locate and replace the current destructive loops in
import_snapshot (the code that calls list_connections, list_saved_queries,
list_ai_providers and then deletes and re-creates them) with the validated
pre-built forms and a single transactional commit/rollback.
In `@src-tauri/src/sync/provider.rs`:
- Around line 80-82: build_provider is currently matching on
config.provider_type which attempts to move ProviderType out of a shared
&SyncConfig; instead match on a reference or clone the enum. Update the match to
use the referenced value (e.g., match &config.provider_type and pattern-match
ProviderType::S3, ProviderType::Gcs, etc.) or explicitly clone the field
(config.provider_type.clone()) before matching so you don't move out of the
borrowed config; reference the build_provider function, the SyncConfig struct
and its provider_type field, and the ProviderType::S3 pattern when making the
change.
In `@src-tauri/src/sync/s3.rs`:
- Around line 63-107: Fix the SigV4 canonical request construction in s3.rs:
build credential_scope as "{date}/{region}/s3/aws4_request" (swap date and
region) instead of "{region}/{date}/s3/aws4_request"; ensure the host header
includes the port for custom endpoints by appending the url.port() when present
(so host header becomes "host:port" when url.port() is Some) instead of using
host_str() alone; and produce canonical and signed headers using lowercase
header names (make signed_headers use k.to_lowercase() and canonical_headers use
k.to_lowercase() as the header name) and ensure headers are sorted by lowercase
key before joining so SignedHeaders matches the canonical_headers order (refer
to variables/methods: credential_scope, signed_headers, canonical_headers, host,
url.port(), and derive_signing_key/signature construction).
- Around line 145-179: The connection test currently probes the bucket root via
a GET (building URL from self.endpoint and self.bucket) which requires
ListBucket; instead, test an object under the configured prefix using a
non-listing method. Modify the code that builds the test URL to include the
configured prefix/object (e.g., append self.prefix or a small probe object name
under the prefix) and change the request to a HEAD (update the sign_request call
to use "HEAD" and replace client.get(...) with client.head(...)); keep using the
empty payload hash and existing signing flow (sign_request, now_timestamps) and
fall back to the bucket-root behavior only if no prefix is configured. Ensure
headers are signed with "HEAD" and the final request uses the head response
status to determine success.
In `@src-tauri/src/sync/scheduler.rs`:
- Around line 9-13: Change the current per-event sleepers into a single
debounced timer that routes all change notifications through shared state so
each new event resets the same pending timer: create within SyncScheduler a
single background debounce task (spawned once in start) that receives
notifications (e.g., via a watch/Notify or an Arc<AtomicBool> + Notify) instead
of spawning a new sleeper per change, have that task await either the debounce
timeout or shutdown_rx (so it cancels on stop), and call try_auto_sync() only
when the timer elapses; also update any code that previously spawned sleepers
(the logic around handling change events and the call sites of try_auto_sync())
to signal the shared notifier/state rather than creating new timers so pending
syncs are cancelled when stop() signals shutdown_rx.
In `@src/components/settings/SyncSettings.tsx`:
- Around line 325-368: The UI hides both password inputs and the save button
when status.enabled && status.passwordStored, preventing persisting provider
changes or password rotation; update the conditional render logic in
SyncSettings (where status, syncPassword, confirmPassword, handleConfigure,
handleSavePassword, and handleTestConnection are used) to always render a save
button for saving provider/config changes even when password is stored/enabled
(e.g., show a "Save" or "Save changes" Button that invokes handleConfigure), and
keep a separate action (e.g., "Rotate password") that invokes handleSavePassword
only when the user wants to update the password; ensure the password inputs
remain hidden when passwordStored is true but provider save remains available.
- Around line 405-433: The force push/pull buttons (rendered with the Button
components calling handleForcePush and handleForcePull) perform destructive
overwrite operations without confirmation; add a confirmation step before
executing: update the onClick handlers for the force push and force pull buttons
to first show a modal or browser confirm dialog that clearly describes the
consequence and requires explicit user confirmation, and only call
handleForcePush or handleForcePull when the user confirms; ensure the
confirmation UI disables the confirm action while loading and preserves existing
loading/error handling.
- Around line 243-319: Hardcoded English strings in the SyncSettings JSX
(provider SelectItem labels, Input placeholders, the "Device ID" label, and the
"min" suffix used with syncInterval options) bypass i18n; replace those literals
with t(...) lookups and interpolations. Update each SelectItem (e.g., the
S3/WebDAV items), all Input placeholders (e.g., endpoint, region, bucket,
serverUrl, username, password, pathPrefix), the Label rendering "Device ID" and
the syncInterval SelectItem labels (e.g., "1 min", "3 min", etc.) to use
translation keys (for example settings.sync.provider.S3,
settings.sync.placeholder.endpoint, settings.sync.deviceId,
settings.sync.intervalLabel with a "{{minutes}} min" interpolation or separate
key for "min") so locales are applied; ensure you call t(...) where
SelectValue/SelectItem and Input.placeholder are defined (components:
SelectItem, Input, Label inside SyncSettings) and update locale files
accordingly.
---
Nitpick comments:
In `@src-tauri/src/sync/scheduler.rs`:
- Around line 15-112: Add source-level unit tests to
src-tauri/src/sync/scheduler.rs to cover the interval clamping, shutdown path,
and change-trigger behavior: create a #[cfg(test)] mod tests in the same file
and add tests that exercise SyncScheduler::get_sync_interval_secs (verify
clamping and default), SyncScheduler::start/stop (spawn the scheduler, send stop
via stop() and assert it exits/shuts down), and notify_data_changed (ensure
try_auto_sync is invoked after debounce) using tokio async tests
(#[tokio::test]) so the async runtime and tokio::time can be controlled (use
tokio::time::pause/advance for timing); mark only long/integration-like tests
with #[ignore] per guidelines and keep tests deterministic by injecting or
mocking the LocalDb/SyncManager behaviors referenced in try_auto_sync / manager
methods.
In `@src/components/settings/SettingsDialog.tsx`:
- Line 58: Update the import in SettingsDialog.tsx to use the repo TypeScript
path alias instead of a relative path; replace the current import of
SyncSettings (import { SyncSettings } from "./SyncSettings";) with the aliased
module import (use "`@/`..." rooted at src) so the component imports follow the
repo-wide convention.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 88b432b6-2514-40a6-9111-d625797c7ace
⛔ Files ignored due to path filters (1)
src-tauri/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (25)
docs/superpowers/plans/2026-06-02-config-sync.mddocs/superpowers/specs/2026-06-02-config-sync-design.mdsrc-tauri/Cargo.tomlsrc-tauri/migrations/017_sync_state.sqlsrc-tauri/src/commands/ai.rssrc-tauri/src/commands/connection.rssrc-tauri/src/commands/mod.rssrc-tauri/src/commands/storage.rssrc-tauri/src/commands/sync.rssrc-tauri/src/db/local.rssrc-tauri/src/lib.rssrc-tauri/src/state.rssrc-tauri/src/sync/crypto.rssrc-tauri/src/sync/manager.rssrc-tauri/src/sync/mod.rssrc-tauri/src/sync/provider.rssrc-tauri/src/sync/s3.rssrc-tauri/src/sync/scheduler.rssrc-tauri/src/sync/webdav.rssrc/components/settings/SettingsDialog.tsxsrc/components/settings/SyncSettings.tsxsrc/lib/i18n/locales/en.tssrc/lib/i18n/locales/ja.tssrc/lib/i18n/locales/zh.tssrc/services/api.ts
Comment on lines
+1065
to
+1070
| // Save config (encrypt sensitive fields) | ||
| let config_json = serde_json::to_string(config) | ||
| .map_err(|e| format!("[SYNC_CONFIG_ERROR] Serialize config: {e}"))?; | ||
|
|
||
| // Store non-sensitive config fields | ||
| db.set_sync_state("sync_config", &config_json).await?; |
There was a problem hiding this comment.
Do not persist provider secrets inside sync_config.
This step serializes the whole SyncConfig, so the S3 secret key / WebDAV password would be written into sync_state in plaintext. That conflicts with the design's “store credentials locally encrypted or outside sync_config” rule and turns the local SQLite file into a cloud-credential leak point.
🤖 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 `@docs/superpowers/plans/2026-06-02-config-sync.md` around lines 1065 - 1070,
The code currently serializes the entire SyncConfig (via serde_json::to_string
into config_json) and persists it with db.set_sync_state("sync_config",
&config_json), which will persist provider secrets in plaintext; instead, filter
out or redact sensitive fields (e.g. S3 secret_key, WebDAV password) before
serializing or replace them with non-secret placeholders, and persist actual
credentials separately using the intended encrypted/secure storage mechanism (or
call the existing credential store API) so that db.set_sync_state("sync_config",
...) only receives a non-sensitive representation of SyncConfig.
Comment on lines
+1131
to
+1142
| // Import remote data if it's from a different device and newer | ||
| if remote_snapshot.device_id != local_device_id { | ||
| self.import_snapshot(&db, &remote_snapshot).await?; | ||
| db.set_sync_state("last_sync_result", "success").await?; | ||
| db.set_sync_state("last_sync_at", &now).await?; | ||
|
|
||
| return Ok(SyncResult { | ||
| action: "pulled".to_string(), | ||
| timestamp: now, | ||
| remote_device_id: Some(remote_snapshot.device_id), | ||
| }); | ||
| } |
There was a problem hiding this comment.
Preserve the timestamp check before importing a remote snapshot.
This imports any snapshot from another device, even when it is older than the local state. The spec's Last-Write-Wins rule requires comparing timestamps first; otherwise sync_now() can overwrite newer local edits with stale remote data.
🤖 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 `@docs/superpowers/plans/2026-06-02-config-sync.md` around lines 1131 - 1142,
The current code imports any snapshot from another device without checking its
timestamp; before calling import_snapshot(&db, &remote_snapshot) (inside the
branch that compares remote_snapshot.device_id != local_device_id) fetch the
local snapshot's timestamp (e.g., via the existing local snapshot lookup or a db
method that returns the latest snapshot timestamp) and compare timestamps per
Last-Write-Wins: only proceed to import if remote_snapshot.timestamp is strictly
newer than the local timestamp; if it's older or equal, skip import, set
appropriate sync state (e.g., "last_sync_result" => "ignored" or "no-op") and
return a SyncResult indicating no pull occurred instead of overwriting newer
local data.
| .cloned() | ||
| .unwrap_or_default(), | ||
| ai_providers: ai_providers_json, | ||
| settings: serde_json::Value::Object(serde_json::Map::new()), // TODO: read from settings.json if needed |
There was a problem hiding this comment.
This plan currently makes settings/shortcuts sync a no-op.
export_snapshot() hardcodes settings to {}, and import_snapshot() explicitly skips applying them. That means the implementation described here would not actually sync user settings or keyboard shortcuts, despite both being in scope for this PR.
Also applies to: 1375-1376
🤖 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 `@docs/superpowers/plans/2026-06-02-config-sync.md` at line 1320,
export_snapshot() currently hardcodes settings to an empty object and
import_snapshot() skips applying settings/shortcuts, so user settings and
keyboard shortcuts never get synced; update export_snapshot() to read and
include the real settings/shortcuts (e.g., load from settings.json or the
settings store used elsewhere) instead of
serde_json::Value::Object(serde_json::Map::new()), and update import_snapshot()
to apply those settings/shortcuts during import (use the same application
logic/handlers that the app uses to persist settings and shortcuts), ensuring
the snapshot payload contains the settings and import_snapshot() processes the
settings and shortcuts entries rather than skipping them.
Comment on lines
+137
to
+148
| ## Database: sync_state Table | ||
|
|
||
| ```sql | ||
| CREATE TABLE IF NOT EXISTS sync_state ( | ||
| key TEXT PRIMARY KEY, | ||
| value TEXT NOT NULL, | ||
| updated_at TEXT NOT NULL DEFAULT (datetime('now')) | ||
| ); | ||
| ``` | ||
|
|
||
| Keys: `device_id`, `sync_config` (JSON, provider params without passwords), `sync_enabled`, `last_synced_hash`, `last_sync_at`, `last_sync_result`, `sync_password_hash` (for verification without storing plaintext) | ||
|
|
There was a problem hiding this comment.
Avoid storing a fast password verifier in sync_state.
A sync_password_hash in local SQLite creates an offline brute-force target if the DB is exposed, and it does not buy you much because decrypting the snapshot already verifies the password. Prefer dropping the verifier entirely, or store a salted slow KDF output instead of a raw hash.
🤖 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 `@docs/superpowers/specs/2026-06-02-config-sync-design.md` around lines 137 -
148, Remove the fast password verifier stored as the key "sync_password_hash" in
the sync_state table and any code paths that read/write that key (references to
sync_state and "sync_password_hash"); either drop the key from the schema and
associated logic, or if a verifier must be kept, replace it with a salted, slow
KDF output (e.g., PBKDF2/Argon2) and update all read/write/verification code to
use the KDF output and salt rather than a raw hash so offline brute-force is not
trivial.
Comment on lines
+13
to
+19
| pub async fn sync_configure( | ||
| state: State<'_, AppState>, | ||
| config: SyncConfig, | ||
| sync_password: String, | ||
| ) -> Result<(), String> { | ||
| let manager = SyncManager::new(state.local_db.clone()); | ||
| manager.configure(&config, &sync_password).await |
There was a problem hiding this comment.
Reject syncIntervalMinutes = 0 before calling configure().
SyncConfig.sync_interval_minutes can currently be 0. SyncManager::configure() persists that raw value and sync_get_status() will report 0, but SyncScheduler::get_sync_interval_secs() clamps it to 1 minute. That leaves the stored config/status out of sync with the actual runtime behavior and can silently increase sync frequency.
Suggested guard
pub async fn sync_configure(
state: State<'_, AppState>,
config: SyncConfig,
sync_password: String,
) -> Result<(), String> {
+ if matches!(config.sync_interval_minutes, Some(0)) {
+ return Err("[SYNC_CONFIG_ERROR] syncIntervalMinutes must be at least 1".to_string());
+ }
let manager = SyncManager::new(state.local_db.clone());
manager.configure(&config, &sync_password).await
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| pub async fn sync_configure( | |
| state: State<'_, AppState>, | |
| config: SyncConfig, | |
| sync_password: String, | |
| ) -> Result<(), String> { | |
| let manager = SyncManager::new(state.local_db.clone()); | |
| manager.configure(&config, &sync_password).await | |
| pub async fn sync_configure( | |
| state: State<'_, AppState>, | |
| config: SyncConfig, | |
| sync_password: String, | |
| ) -> Result<(), String> { | |
| if matches!(config.sync_interval_minutes, Some(0)) { | |
| return Err("[SYNC_CONFIG_ERROR] syncIntervalMinutes must be at least 1".to_string()); | |
| } | |
| let manager = SyncManager::new(state.local_db.clone()); | |
| manager.configure(&config, &sync_password).await | |
| } |
🤖 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 `@src-tauri/src/commands/sync.rs` around lines 13 - 19, Validate
SyncConfig.sync_interval_minutes in sync_configure and reject values equal to 0
before calling SyncManager::configure: check config.sync_interval_minutes == 0
and return an Err with a clear message (e.g. "sync_interval_minutes must be >=
1") so the invalid value is not persisted; reference sync_configure,
SyncConfig.sync_interval_minutes, SyncManager::configure,
SyncScheduler::get_sync_interval_secs, and sync_get_status when making the
change.
Comment on lines
+145
to
+179
| let url: url::Url = format!("{}/{}/", self.endpoint, self.bucket) | ||
| .parse() | ||
| .map_err(|e: url::ParseError| { | ||
| format!("[SYNC_CONNECTION_ERROR] Invalid URL: {e}") | ||
| })?; | ||
|
|
||
| let empty_payload_hash = hex::encode(sha256(b"")); | ||
| let (date, datetime) = Self::now_timestamps(); | ||
|
|
||
| let mut headers = vec![( | ||
| "Content-Type".to_string(), | ||
| "application/octet-stream".to_string(), | ||
| )]; | ||
| self.sign_request("GET", &url, &mut headers, &empty_payload_hash, &date, &datetime); | ||
|
|
||
| let mut req = self.client.get(url.as_str()); | ||
| for (k, v) in &headers { | ||
| req = req.header(k.as_str(), v.as_str()); | ||
| } | ||
|
|
||
| let resp = req | ||
| .send() | ||
| .await | ||
| .map_err(|e| format!("[SYNC_CONNECTION_ERROR] {e}"))?; | ||
| let status = resp.status(); | ||
| if status.is_success() { | ||
| Ok(()) | ||
| } else { | ||
| let body = resp.text().await.unwrap_or_default(); | ||
| Err(format!( | ||
| "[SYNC_CONNECTION_ERROR] S3 returned {}: {}", | ||
| status, | ||
| body.chars().take(200).collect::<String>() | ||
| )) | ||
| } |
There was a problem hiding this comment.
Bucket-root GET makes test_connection() stricter than the real sync operations.
This probes GET /{bucket}/, which typically needs ListBucket permission. A credential set scoped only to the configured prefix/object can successfully PUT/GET/DELETE the snapshot but will still fail this test, so valid least-privilege configs get rejected up front.
🤖 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 `@src-tauri/src/sync/s3.rs` around lines 145 - 179, The connection test
currently probes the bucket root via a GET (building URL from self.endpoint and
self.bucket) which requires ListBucket; instead, test an object under the
configured prefix using a non-listing method. Modify the code that builds the
test URL to include the configured prefix/object (e.g., append self.prefix or a
small probe object name under the prefix) and change the request to a HEAD
(update the sign_request call to use "HEAD" and replace client.get(...) with
client.head(...)); keep using the empty payload hash and existing signing flow
(sign_request, now_timestamps) and fall back to the bucket-root behavior only if
no prefix is configured. Ensure headers are signed with "HEAD" and the final
request uses the head response status to determine success.
Comment on lines
+9
to
+13
| pub struct SyncScheduler { | ||
| local_db: Arc<Mutex<Option<Arc<crate::db::local::LocalDb>>>>, | ||
| shutdown_tx: watch::Sender<bool>, | ||
| shutdown_rx: watch::Receiver<bool>, | ||
| } |
There was a problem hiding this comment.
Coalesce change notifications into a single debounce window.
This delays each event, but it does not actually debounce them. A burst of writes will spawn several sleepers that can all reach try_auto_sync() together, and those pending tasks still run after stop() because they never observe the shutdown channel. Please route change events through shared state so each new event resets one pending timer instead of starting another sync attempt.
Also applies to: 54-61
🤖 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 `@src-tauri/src/sync/scheduler.rs` around lines 9 - 13, Change the current
per-event sleepers into a single debounced timer that routes all change
notifications through shared state so each new event resets the same pending
timer: create within SyncScheduler a single background debounce task (spawned
once in start) that receives notifications (e.g., via a watch/Notify or an
Arc<AtomicBool> + Notify) instead of spawning a new sleeper per change, have
that task await either the debounce timeout or shutdown_rx (so it cancels on
stop), and call try_auto_sync() only when the timer elapses; also update any
code that previously spawned sleepers (the logic around handling change events
and the call sites of try_auto_sync()) to signal the shared notifier/state
rather than creating new timers so pending syncs are cancelled when stop()
signals shutdown_rx.
Comment on lines
+243
to
+319
| <SelectItem value="S3">S3 (AWS / MinIO / OSS)</SelectItem> | ||
| <SelectItem value="WebDAV">WebDAV</SelectItem> | ||
| </SelectContent> | ||
| </Select> | ||
|
|
||
| {providerType === "S3" ? ( | ||
| <div className="space-y-2"> | ||
| <Input | ||
| placeholder="Endpoint (e.g., https://s3.amazonaws.com)" | ||
| value={endpoint} | ||
| onChange={(e) => setEndpoint(e.target.value)} | ||
| /> | ||
| <Input | ||
| placeholder="Region (e.g., us-east-1)" | ||
| value={region} | ||
| onChange={(e) => setRegion(e.target.value)} | ||
| /> | ||
| <Input | ||
| placeholder="Bucket" | ||
| value={bucket} | ||
| onChange={(e) => setBucket(e.target.value)} | ||
| /> | ||
| <Input | ||
| placeholder="Access Key ID" | ||
| value={accessKeyId} | ||
| onChange={(e) => setAccessKeyId(e.target.value)} | ||
| /> | ||
| <Input | ||
| placeholder="Secret Access Key" | ||
| type="password" | ||
| value={secretAccessKey} | ||
| onChange={(e) => setSecretAccessKey(e.target.value)} | ||
| /> | ||
| <Input | ||
| placeholder="Path Prefix (default: dbpaw/)" | ||
| value={pathPrefix} | ||
| onChange={(e) => setPathPrefix(e.target.value)} | ||
| /> | ||
| </div> | ||
| ) : ( | ||
| <div className="space-y-2"> | ||
| <Input | ||
| placeholder="Server URL (e.g., https://dav.example.com/dbpaw/)" | ||
| value={serverUrl} | ||
| onChange={(e) => setServerUrl(e.target.value)} | ||
| /> | ||
| <Input | ||
| placeholder="Username" | ||
| value={username} | ||
| onChange={(e) => setUsername(e.target.value)} | ||
| /> | ||
| <Input | ||
| placeholder="Password" | ||
| type="password" | ||
| value={password} | ||
| onChange={(e) => setPassword(e.target.value)} | ||
| /> | ||
| </div> | ||
| )} | ||
|
|
||
| <Separator className="my-2" /> | ||
|
|
||
| <Label className="text-base">{t("settings.sync.syncInterval")}</Label> | ||
| <Select | ||
| value={String(syncInterval)} | ||
| onValueChange={(v) => setSyncInterval(Number(v))} | ||
| > | ||
| <SelectTrigger> | ||
| <SelectValue /> | ||
| </SelectTrigger> | ||
| <SelectContent> | ||
| <SelectItem value="1">1 min</SelectItem> | ||
| <SelectItem value="3">3 min</SelectItem> | ||
| <SelectItem value="5">5 min</SelectItem> | ||
| <SelectItem value="10">10 min</SelectItem> | ||
| <SelectItem value="30">30 min</SelectItem> | ||
| <SelectItem value="60">60 min</SelectItem> |
There was a problem hiding this comment.
Several new sync strings bypass i18n.
The provider option labels, input placeholders, Device ID, and min suffix are hardcoded in English, so the new sync page will still be partially untranslated in ja and zh even though locale files were added.
Also applies to: 331-346, 389-403
🤖 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 `@src/components/settings/SyncSettings.tsx` around lines 243 - 319, Hardcoded
English strings in the SyncSettings JSX (provider SelectItem labels, Input
placeholders, the "Device ID" label, and the "min" suffix used with syncInterval
options) bypass i18n; replace those literals with t(...) lookups and
interpolations. Update each SelectItem (e.g., the S3/WebDAV items), all Input
placeholders (e.g., endpoint, region, bucket, serverUrl, username, password,
pathPrefix), the Label rendering "Device ID" and the syncInterval SelectItem
labels (e.g., "1 min", "3 min", etc.) to use translation keys (for example
settings.sync.provider.S3, settings.sync.placeholder.endpoint,
settings.sync.deviceId, settings.sync.intervalLabel with a "{{minutes}} min"
interpolation or separate key for "min") so locales are applied; ensure you call
t(...) where SelectValue/SelectItem and Input.placeholder are defined
(components: SelectItem, Input, Label inside SyncSettings) and update locale
files accordingly.
Comment on lines
+325
to
+368
| {(!status?.enabled || !status?.passwordStored) && ( | ||
| <> | ||
| <Label className="text-base"> | ||
| {t("settings.sync.syncPassword")} | ||
| </Label> | ||
| <Input | ||
| placeholder="Sync password (min 6 chars)" | ||
| type="password" | ||
| value={syncPassword} | ||
| onChange={(e) => setSyncPassword(e.target.value)} | ||
| /> | ||
| {!status?.enabled && ( | ||
| <Input | ||
| placeholder="Confirm password" | ||
| type="password" | ||
| value={confirmPassword} | ||
| onChange={(e) => setConfirmPassword(e.target.value)} | ||
| /> | ||
| )} | ||
| {status?.enabled && !status?.passwordStored && ( | ||
| <div className="text-xs text-muted-foreground"> | ||
| {t("settings.sync.passwordNotStored")} | ||
| </div> | ||
| )} | ||
| </> | ||
| )} | ||
|
|
||
| <div className="flex gap-2 mt-2"> | ||
| <Button | ||
| variant="outline" | ||
| onClick={handleTestConnection} | ||
| disabled={loading} | ||
| > | ||
| {t("settings.sync.testConnection")} | ||
| </Button> | ||
| {!status?.enabled ? ( | ||
| <Button onClick={handleConfigure} disabled={loading}> | ||
| {t("settings.sync.saveAndEnable")} | ||
| </Button> | ||
| ) : !status?.passwordStored ? ( | ||
| <Button onClick={handleSavePassword} disabled={loading}> | ||
| {t("settings.sync.saveAndEnable")} | ||
| </Button> | ||
| ) : null} |
There was a problem hiding this comment.
Enabled sync becomes impossible to edit from the UI.
When status.enabled && status.passwordStored is true, this branch hides both the password inputs and the only save button, so provider changes made in the form can never be persisted and users have no path to rotate the sync password without disabling sync first.
🤖 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 `@src/components/settings/SyncSettings.tsx` around lines 325 - 368, The UI
hides both password inputs and the save button when status.enabled &&
status.passwordStored, preventing persisting provider changes or password
rotation; update the conditional render logic in SyncSettings (where status,
syncPassword, confirmPassword, handleConfigure, handleSavePassword, and
handleTestConnection are used) to always render a save button for saving
provider/config changes even when password is stored/enabled (e.g., show a
"Save" or "Save changes" Button that invokes handleConfigure), and keep a
separate action (e.g., "Rotate password") that invokes handleSavePassword only
when the user wants to update the password; ensure the password inputs remain
hidden when passwordStored is true but provider save remains available.
Comment on lines
+405
to
+433
| {status.enabled && status.passwordStored && ( | ||
| <div className="mt-2 flex gap-2"> | ||
| <Button | ||
| size="sm" | ||
| variant="outline" | ||
| onClick={handleSyncNow} | ||
| disabled={loading} | ||
| > | ||
| <RefreshCw className="w-3.5 h-3.5 mr-1" /> | ||
| {t("settings.sync.syncNow")} | ||
| </Button> | ||
| <Button | ||
| size="sm" | ||
| variant="outline" | ||
| onClick={handleForcePush} | ||
| disabled={loading} | ||
| > | ||
| <Upload className="w-3.5 h-3.5 mr-1" /> | ||
| {t("settings.sync.forcePush")} | ||
| </Button> | ||
| <Button | ||
| size="sm" | ||
| variant="outline" | ||
| onClick={handleForcePull} | ||
| disabled={loading} | ||
| > | ||
| <Download className="w-3.5 h-3.5 mr-1" /> | ||
| {t("settings.sync.forcePull")} | ||
| </Button> |
There was a problem hiding this comment.
Add confirmation before force push/pull.
These buttons trigger overwrite-style operations immediately. A misclick here can replace either the remote snapshot or the local config with no confirmation step.
🤖 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 `@src/components/settings/SyncSettings.tsx` around lines 405 - 433, The force
push/pull buttons (rendered with the Button components calling handleForcePush
and handleForcePull) perform destructive overwrite operations without
confirmation; add a confirmation step before executing: update the onClick
handlers for the force push and force pull buttons to first show a modal or
browser confirm dialog that clearly describes the consequence and requires
explicit user confirmation, and only call handleForcePush or handleForcePull
when the user confirms; ensure the confirmation UI disables the confirm action
while loading and preserves existing loading/error handling.
Owner
|
@mrhuangyong 感谢感谢,我今晚review一下再合入master |
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
2 participants
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 by CodeRabbit
New Features
Documentation