Make Comet local-first with optional multi-device sync - #30
Conversation
|
I'll resolve conflicts now |
There was a problem hiding this comment.
Review findings and Linux validation
Reviewed head 9a775fc890f4f592fbaca4c3355ade546074c94d.
Findings
-
Local-only still contacts Edge.
assemble_runtimestarts the updater regardless ofWorkspaceScopeor the computedonlineflag. After the initial delay, the updater callsfetch_latest, which requests the release manifest and fallback version file. This conflicts with the clean-install “without contacting Edge” behavior described by the PR. Please gate update checks on online scope or explicit update consent. -
Synced-profile attachment isolation is not account-scoped. All Synced and Development profiles still use
{data_dir}/uploads, and the attachment jail trusts the entire configured upload root. Switching accounts therefore leaves known attachment paths, staging IDs, and commit filenames accessible or collidable across profiles. Please use account-scoped upload roots and define a migration/read-only fallback for the legacy cache. -
Engine assembly failures can leave the UI falsely Ready. The embedded engine is assembled in a detached task, but
attach_enginemarks the connection Ready immediately. If store or journal assembly later entersDeferredEngineState::Failed, the standing watches log the subscription failure and return. A corrupt or inaccessible local store can therefore show an empty ready application with no error/retry path. Please propagate deferred failure intoConnectionStatus::Failed, or await local assembly before publishing Ready. -
The systemd crash-loop limit was removed while
Restart=on-failureremains. Both the generated unit and installer unit now restart every five seconds without the previous explicit rate limit. Permanent errors such as a corrupt store, invalid environment, or bind failure will churn indefinitely. Please restore an explicit start limit.
Device identity locking
The short device-identity lock is useful because identity resolution and runtime assembly can race before the lifetime engine lock. A kernel-owned file lock also recovers automatically after a crash.
The raw unsafe { libc::flock(...) } call is not needed on the workspace's rolling stable Rust toolchain. Safe cross-platform std::fs::File::{lock, try_lock, unlock} has been stable since Rust 1.89. The blocking identity lock can use file.lock()?; the lifetime instance lock can use try_lock() while preserving its retry and error handling.
Validation
- Workspace test suite with
comet-sync/mock-server: passed, with the two documented baseline tests skipped cargo test -p comet-rpc --example e2e_driver: passed- Workspace Clippy across all targets: passed with existing warnings
git diff --check: passed- Headed Linux runtime: clean local boot, local workspace operation, restart persistence, optional-sync UI, login UI, and Devices settings all rendered successfully
cargo fmt --all -- --check: the PR-touched failure is import ordering incrates/ui/src/shell.rs; the other reported files are outside this PR
UI captures
Clean local start:
Local account menu with the optional sync action:
Enable Sync browser handoff:
Login gate:
Local Devices settings:
|
thanks a lot for this PR, i did a cursory read and had codex go through this on a linux machine nit: i think enable sync should be above settings @SinaKhalili can you take a look as well please |





I tested this extensively on macOS, but I haven't been able to test it on Linux or Windows since I don't currently have environments set up for those platforms.
Please review this carefully, as these changes affect important parts of the app. I've also extended the E2E tests to improve macOS coverage.
Feel free to request any changes. I'll keep an eye out for feedback and address anything that comes up.
Closes #3
-- The following is AI-- :
Summary
What changed
Local-first engine profiles
Local,Synced, andDevelopmentworkspace scopes, resolve the active profile once at startup, and expose it through the newEngineInfoRPC.profiles/local/with a stable installation-local identity.orgs/{org_id}/{user_id}layout for authenticated profiles, so current synced installations reopen their existing data in place.Authentication and lifecycle
comet headlessand the Linux installer start immediately in local-only mode; signing in is no longer required to run the daemon.comet loginandcomet logoutselect the profile for the next engine start, while refusing to mutate credentials behind a running engine.comet status, with signed-out local mode treated as healthy.Desktop experience
EngineInfoas synced so they retain the previous authentication gate instead of being mistaken for local runtimes.Tests and documentation
Upgrade behavior
Testing
cargo test --workspace --features comet-sync/mock-server -- --skip repos_round_trip_add_branches_worktrees --skip falls_back_when_interactive_attempt_hangscargo test -p comet-rpc --example e2e_drivercargo clippy --workspace --all-targets --features comet-sync/mock-serverscripts/e2e-smoke.shgit diff --check upstream/main...HEADThe changes introduced by this branch pass Clippy. The workspace Clippy command exits successfully with only pre-existing warnings outside the modified hunks:
collapsible_ifincrates/update; fourcloned_ref_to_slice_refswarnings incrates/doc;needless_return, twoobfuscated_if_elsewarnings, and one unused test variable incrates/engine; and, incrates/ui, sevenunexpected_cfgswarnings from the macOS Objective-C macros plusmanual_is_multiple_of,collapsible_if,question_mark,unnecessary_sort_by, fourdoc_lazy_continuationwarnings, threetype_complexitywarnings,too_many_arguments, andlen_without_is_empty. Running Clippy with-D warningstherefore stops at the first unchangedcollapsible_ifwarning incrates/update/src/lib.rs. The workspace test command skips two unchanged baseline tests on macOS: one compares/var/...with its canonical/private/var/...path, and one is an existing interactive-shell timeout flake.cargo fmt --all -- --checkcurrently reports only pre-existing formatting drift in unchangedcrates/sync/src/room.rs, which is identical toupstream/main.Need help on this PR? Tag
@codesmith-botwith what you need. Autofix is disabled.