Skip to content

Commit c05e275

Browse files
authored
feat(profiles): one server per purpose, with a Server panel you can read (SPEC-50) (#162)
* Say why the daemon would not start (SPEC-50 P0) Every daemon start/stop/restart failure was invisible. Two defects stacked: `makit start` reports why it failed on stdout, not stderr -- the daemon is spawned detached with its output redirected into the log, so the parent's only diagnostic is deps.out() -> console.log. The app read res.stderr alone, so a real failure surfaced as the bare string "makit start exited 1: ". Worse, DaemonActionResult was discarded by all four call sites, so even a correct message went nowhere. _failureMessage() now reads both streams (stderr first, it carries the lower-level cause), dedupes, and drops the separator when both are empty so the message can never end in a dangling colon. All four call sites post a StatusCenter failure. Proven on a real port collision: exit 1, reason on stdout, stderr empty. Also adds SPEC-50 and its mockup (12 cards) as ground truth for the rest. * Profiles: one server per purpose (SPEC-50 P1-P4) A profile is now a thing the user owns rather than plumbing derived from a file path: it can be named, listed, started, stopped, and deleted, and several run at once. Proven live -- two daemons on 7861/7862 with separate MAKIT_HOMEs, each minting its own TLS fingerprint, which is what makes per-profile QR pairing work. Identity is persisted. ProfileRegistry mints the id once into ~/.makit/profiles.json and re-binds a rebuilt or moved dev build by its stored origin. Before this, id = fnv1a(repoRoot) was recomputed every launch, so moving a worktree minted a NEW profile and orphaned the old one's home, pairings, projects and prefs. Measured on this machine: 27 of 33 dev homes (732 MB) were already unreachable that way, each still holding a device pairing and a TLS keypair. They are now listed, sized, and offered for bulk reclaim -- offered, never reaped, because auto-deletion would also have destroyed transcripts the first time a worktree moved. isDefault split into name (a UI fact, editable) and storage (legacy|namespaced, a compatibility fact, frozen). storage:legacy pins the shipped key layout and implies protected, so ~/.makit is renameable yet undeletable and no shipped user's prefs move -- the effective key stays byte-identical, asserted by test. Ports are allocated by probing upward from the hash guess and persisted, skipping ports other profiles claim, since a probe cannot see a stopped profile's port. 100 slots for unbounded worktrees made collisions inevitable and nothing reallocated. Server & Devices drops from ~10 controls to four rows: active profile, one reachability question, pair-a-phone, and a Diagnostics disclosure holding pid/CLI/fingerprint/log plus Advanced. The two-phase "Save & restart server" is gone. Four exclusive bind modes become Reachability{thisMacOnly,myDevices} plus allowLanFallback, because Auto and LAN were one behaviour wearing two labels: chooseBindHost prefers Tailscale before it consults allowLan, so --lan is a fallback. chooseBindHost is unchanged. Every persisted generation migrates, and the newest wins when all three are present -- otherwise a stale bind mode would re-open a server the user had restricted. Deleting a profile erases all four stores (home dir, secure-store namespace, prefs, registry entry last -- omit the entry and it resurrects empty) and says what it KEEPS: worktrees and repos are never touched. It refuses rather than throws, and the refusals are the interesting part. Three findings from review, all reproduced before fixing: - Orphan reclaim could never work. stopAndConfirm polled the control socket FILE, but a SIGKILLed daemon never unlinks it and `makit stop` on a dead daemon removes only the pid file -- verified against the real binary. So every crashed profile looked alive forever, and exactly the orphans this feature exists to reclaim were permanently undeletable. It now polls liveness. - A rogue registry entry could erase the APNs key. The guard trusted the entry's own storage flag, but profiles.json is user-writable. Fixed once, then defeated again by a single trailing slash (~/.makit/ is not == to ~/.makit yet satisfies a startsWith check). Paths are now canonicalised before any comparison, and a fourth rule that mutation testing showed could never bite was deleted rather than left as reassurance. - Concurrent instances could lose a profile. save() now re-reads and merges by id, remembers deletions so a merge cannot resurrect them, and uses a pid-suffixed temp file. profiles.json is written 0600 inside a 0700 directory, matching the guarantee the server makes for a directory holding an APNs auth key and a TLS private key; Dart's defaults would have left it 0644 in 0755. D10 (in-place switching) is deferred, with the reason and the exact remaining refactor written into the spec. Its foundations are here and tested (ProfileScopedPrefs, verified byte-identical to the setPrefix keys it replaces), but adopting them across WorkspaceController's 20 files could not be done safely in one pass, and a partial adoption would switch the server while still showing another profile's panes -- a subtler failure than not switching at all. The badge therefore names the active profile (it used to hide it) but does not yet change it. Also: the pair QR carries optional &n= and &id= so a phone can label each server instead of showing a bare IP, byte-identical when absent; Install CLI moved to General. app 2256 tests pass, 0 real failures (loading-stage flakes are pre-existing and random); analyze clean under --fatal-infos; format clean. server 1283 tests pass, tsc clean. * Profiles: scope prefs per profile, and stop listing dead profiles twice Prefs (SPEC-50 D11). ServerConfigController and GroupsController now take a ScopedPrefs instead of SharedPreferences, and desktop_app no longer calls SharedPreferences.setPrefix -- which throws once getInstance() has run and so blocked in-place switching outright. Keys are composed by us and compose byte-identically, so this is a no-op rather than a migration. Appearance, shortcuts, recent models and cached commands become shared across profiles, which is intended: the old blanket prefix is why a worktree build opened with a default theme and empty shortcuts. Correction to the deferral rationale: WorkspaceController holds NO preferences (its only mention of SharedPreferences is a doc comment); pane layouts persist through GroupsController. My earlier '20 files / 154 references' figure counted files that merely mention the class, so D10 was materially cheaper than claimed. UI, found by rendering it. Built tool/profiles_demo.dart and captured the real macOS window with cua-driver: stale profiles appeared TWICE, once as ordinary rows and once in the stale group. On this machine that is 5 of 9 rows -- and 27 of 33 on a real one -- so dead profiles crowd the live ones off the screen. The main list now excludes them; the stale group is their only home. tool/profiles_demo.dart renders both surfaces from in-memory fakes with every delete refused, so a design review cannot touch real data. app 1108 tests pass in the touched trees, 0 real failures; analyze clean. * Profiles: switch in the window you are already in (SPEC-50 D10) The last gap. Picking a profile from the title-bar pill now confirms, verifies the target, and hands the window over -- no relaunch. ProfileRuntime holds the whole per-profile object graph (control client, daemon controller, scoped prefs controllers, lifecycle, deleter) behind one disposable, so a second one can exist. Switching is a key change on the ProviderScope, which makes Riverpod dispose the entire old container -- there is no hand-written teardown list to forget an entry later. Dispose cancels the poll timer before closing the client, because a poll firing against a closed client throws. Order is the point: the target is started and confirmed answering WHILE the current profile is still live. If it cannot come up, nothing changes and the reason is reported. Success and failure both surface through StatusCenter, so a switch can never silently not happen. The tray was the trap. It closed over one DesktopController, so after a switch the menubar would drive a disposed object; it now reads through a holder and its listener is re-attached on each switch. The pill became the switcher and degrades gracefully: many surfaces (and most widget tests) mount the badge with no profile wiring, so both new providers default to null and the badge falls back to a calm label rather than crashing -- which is also the honest UI when there is nothing to switch to. That kept ~50 sidebar tests untouched. 'Switch away & delete' is now a real action instead of a disabled item with an apology. Last-active profile persists in profiles.json, honoured ONLY for the installed app: a dev build always opens its own profile, or building a worktree would silently reopen Work and look like the build did nothing. The spec records the correction rather than hiding it: I deferred D10 claiming WorkspaceController needed a 20-file refactor, but it holds no preferences at all -- I had counted files that merely mention the class. app 2195 pass, 0 real failures; analyze clean under --fatal-infos; format clean. server 1283 pass, tsc clean. * Profiles: make 'switch away & delete' actually switch, and test the switch Two defects in the previous commit, both found by asking whether it was really finished. 'Switch away & delete' did not switch. I enabled the menu item but left it calling the delete sheet directly, so for the ACTIVE profile ProfileDeleter refused it every time -- an honest disabled item replaced by a broken promise. It now picks a landing profile (preferring the protected one, which always exists), confirms both consequences in ONE sheet because it is one intent, and hands the delete to the host: the widget offering it cannot do the work, because the ProviderScope it lives in is disposed by the switch. The host survives that rebuild and deletes through the NEW runtime's deleter, which correctly sees the old profile as inactive. The switch sequence had no tests and had never executed. Extracted it as verifyThenHandOver, where the irreversible half is injected, so the property that matters is assertable: does it hand the window over at all? Eight tests now cover a running target (no spawn), a stopped one (start then hand over), a target that refuses to start, and -- the subtler case -- one where makit: already running makit: running pid 82738 listening 100.119.58.97:7808 fingerprint d3e73b15ac380e583d588c5f8d7fa1b8a097d10c853cdbd34ecc4f890f088651 paired 8 device(s) sessions 3 running uptime 138373s version 0.0.0 exits 0 but nothing is listening, where trusting the exit code would hand the window to a dead server. All three guards bite under mutation (1, 1 and 2 failures). That test also caught a weak message: with empty CLI output the failure read 'makit start exited 1' and never named the profile, which is useless where the detail is surfaced alone. It always names the profile now. ProfilesController gains notifyRegistryChanged, because the host mutates the shared registry directly and notifyListeners is protected to subclasses. app 2278 pass, 0 real failures; analyze clean under --fatal-infos; format clean. * Profiles: cover the confirm sheets, and correct a misleading guard comment Both findings from an open-code-review pass over the three unreviewed commits. The switch sheets had no test file. confirmProfileSwitch was exercised only incidentally through the badge, and confirmSwitchAwayAndDelete -- which gates a delete that erases a profile's database, media, pairings and TLS identity -- had no coverage at all. Twelve tests now cover both: that they name the target, that they carry BOTH halves of the consequence, that a start is promised only for a stopped target, and that Cancel and outside-tap dismissal each return false. That last one matters most: a dismissed dialog pops null, and reading null as consent would switch or delete on a stray click. Mutations bite -- 'result ?? true' fails 2, always-promising-a-start fails 1, renaming the kept block fails 1. The demo harness comment was wrong about its own safety mechanism. It claimed every delete is refused because everything is 'active', but activeProfileId 'ALL-REFUSED' matches nothing: a namespaced profile like 'personal' would pass both that check and isProtected. The real guard is homeDir '/nonexistent', which makes _unsafeHomeReason reject every seeded home. The comment now says so, since a guard nobody understands is one somebody will remove. Note the review itself was PARTIAL: 7 of 21 files failed in the run that produced these findings (19 of 21 in a first attempt), so this is not a clean bill of health for the diff -- only for what was actually read. app 2272 pass, 0 real failures; analyze clean under --fatal-infos; format clean. * fix(profiles): address Macroscope review findings (SPEC-50) Correctness fixes from the PR #162 review, each with a regression test: - server_profile: reject out-of-range persisted ports (>65535) so a hand-edited profiles.json falls back instead of silently failing to bind. - profile_registry: serialise save()'s read-merge-write under an inter-process advisory lock so two instances can't drop each other's new profile; preserve a newer on-disk lastActive unless this instance set it. - profile_deleter: guard diskUsage() with a containment check (a corrupt home:"/" no longer walks the whole disk) and make the size walk resilient to unreadable dirs; make delete() best-effort so a store failure is recorded and always yields a result instead of a partial delete. - profile_lifecycle: stopAndConfirm now waits for the daemon PID to exit, not just the control socket, closing the window where MAKIT_HOME could be deleted under a still-live daemon writing makit.db-wal. - reconnecting_control_client: close() awaits an in-flight connect so it can't install and leak a live socket after teardown. - desktop_app: serialise profile switches at switchTo() so two interleaved switches can't leave the window, persistence and title disagreeing. - profiles_section: omit/disable Start-Stop for the active profile (never stop the daemon this window talks to); add profiles.* deep-link anchors. - profile_delete_sheet / reclaim_sheet: stop claiming prefs are deleted when they're skipped; wrap deletions in try/catch so one failure can't abort the flow. - server_config / server_devices_section: setReachability clears customHost so the choice takes effect; the Address line reflects the effective bind rather than the client's loopback connection. - server_devices_section: auto-expand Diagnostics when a deep-link targets one of its (otherwise unmounted) rows. * fix(profiles): address second Macroscope review round (SPEC-50) Nine more review findings, each with a regression test: - server_profile: reject a relative `home` (must be an absolute MAKIT_HOME so spawned CLIs don't resolve it against their cwd). - profile_registry: harden multi-instance save()/load() — - honour deletions across windows via persisted `deletedIds` tombstones so a stale window can't resurrect a profile whose stores are already erased; - only override an on-disk profile this instance actually modified (tracked like `_deleted`), so an unrelated save no longer reverts another window's rename/port/origin edit; - reconcile duplicate ports after the merge (and on load), so two instances that independently allocate the same free port — or a hand-edited/fallback 7777 that collides with the legacy profile — don't hit EADDRINUSE; - drop a second `legacy` profile on load (D2: at most one may own the unprefixed prefs keys and unsuffixed secure store). - profile_lifecycle: stopAndConfirm returns false if `stop()` itself failed (CLI missing / command failed) instead of proceeding as if shut down. - profile_deleter (Critical): resolve symlinks before the containment check so a symlinked ancestor (e.g. ~/.makit/profiles -> external) can't route a recursive delete outside ~/.makit*; both sides are resolved to avoid false positives on symlinked temp roots. - profiles_section: the active profile's danger-zone button now runs switch-away-&-delete (it was permanently disabled despite its label). * fix(profiles): third review round + run live safety tests in CI - profile_registry: _uniqueId now avoids tombstoned ids too. Reusing a just- deleted slug made save() drop the new profile (it honours the tombstone) and orphan its home; minting a fresh id keeps it persistable. - profiles_controller / profiles_section: keep an active profile in the main list even when it is stale, and exclude it from the reclaim group (whose deleter refuses the active profile). Otherwise a stale active profile had no reachable delete at all; now its row's switch-away-&-delete works. - profile_deleter_live_test: moved from app/lib/ to app/test/ so `flutter test` (which only scans test/) actually runs the real-filesystem deletion-safety checks (symlink escape, traversal, legacy-home protection) in CI. - profile_lifecycle_test: inject statusProbe in the socket-disappearance test so isRunning is genuinely driven by socketExists and the polling path is exercised, instead of short-circuiting on the default probe. Regression tests added for the tombstoned-id reuse and the active-stale-profile cases. * fix(profiles): address fourth Macroscope review round (SPEC-50) * fix(profiles): finish the fourth review round (SPEC-50) Completes the CodeRabbit/Macroscope round begun in 288b7b2. - desktop_controller: serialize start/stop/restart. They share the daemon's PID file and control socket (and restart is a stop+start), so overlapping requests could drop a fresh PID, launch a second daemon, or stop one another action had just started. Each action now chains after the previous, with a regression test asserting they never overlap. - profile_lifecycle: keep the pid-file read synchronous. Making it async added real filesystem microtasks that a widget test's pumpAndSettle can never settle, which hung the delete tests. Only `processAlive` (polled ~100x per stop) needed to be async, and it stays async. - test doubles: override `withLock` in the two "writes nothing" FileSystemAdapter fakes. They inherited the real implementation, which created `<path>.lock` on the real disk under a non-existent /Users/test — the actual cause of the hang. - profiles_section_test: replace `await Future.delayed(...)` with `tester.pump(...)`; inside testWidgets the clock only advances when pumping, so the bare delay deadlocked. - profile_registry: keep the already-persisted profile's port when reconciling a post-merge collision (its daemon may be running on it) and reassign the newcomer instead; test asserts which side moves. - profile_runtime_test: assert dispose() disposes profilesController. - server_devices_section_test: reuse `_pump` via an optional NavigatorObserver. - docs/mockup: record the 64-code-point cap on the pair-URL `n` param, fix the delta cross-references (18→19, 17→18), use `pnpm typecheck` as the documented server check, and correct two stale "explicitly not changed" claims. * fix(profiles): fourth review round (CLI path, prefs purge, guards) * fix(profiles): per-profile lifecycle config, real prefs purge (SPEC-50) Four more review findings: - profile_lifecycle / profile_runtime: lifecycle actions target arbitrary profiles, but the CLI path and endpoint args came from the ACTIVE profile's config. Starting profile B from A's window therefore used A's configured binary and, since `start` passed no `--port`, launched B's daemon on the CLI default (7777) — colliding with the legacy daemon or landing on an endpoint B's own ServerConfig disagreed with. ProfileLifecycle now takes `cliPathFor` and `serveArgsFor`, wired to the target's scoped ServerConfig, and MakitCliResolver.resolve() accepts a per-call override. - profile_deleter: actually purge the profile's preference keys (store 3). The unconditional skip was based on the obsolete `SharedPreferences.setPrefix` assumption; prefs are now scoped by key prefix, so `ProfileScopedPrefs .clearScope()` can purge a non-active profile. Injected as `purgePrefs`, so contexts with no prefs still report the store honestly. The delete sheet lists prefs under "Will be deleted" again, and its caveat note is gone. - profile_deleter: a regular file at `home` is no longer reported as removed while `deleteDirectory` silently no-ops on it — `ProfileFileSystem` gains `isDirectory`, and the file case is erased with `deleteFile` and named in the result. - profiles_section: `promptCreateProfile` catches a throwing `create` (unwritable registry, failed port allocation) and reports it, instead of leaking an unhandled async error with no user feedback. * fix(profiles): report and revert a rename whose save fails (SPEC-50) `promptRenameProfile` called `controller.rename` without catching persistence failures. `rename` mutates the registry's in-memory list and then saves, so an unwritable registry left the row showing the new name for the rest of the session while the change was silently lost on restart — and the exception escaped as an unhandled async error with no user feedback. It now reports the failure and reverts the in-memory name (without saving) so the row shows what is actually persisted. Regression test drives Rename through a registry whose writes throw and asserts both the failure event and the revert.
1 parent a3d3da5 commit c05e275

55 files changed

Lines changed: 11146 additions & 887 deletions

File tree

Some content is hidden

Large Commits have some content hidden by default. Use the searchbox below for content that may be hidden.

.gitignore

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -24,7 +24,10 @@ app/test/sim/frames/
2424

2525
# Local QA artifacts (screenshots, issue logs) from tool/shoot-ports.sh
2626
.qa/
27-
.piano/
27+
28+
# Editor/agent scratch files must never be committed.
29+
*.bak
30+
*.orig
2831

2932
# Local pnpm store, created when a store-dir is set for this checkout
3033
.pnpm-store/

app/integration_test/desktop/control_e2e_test.dart

Lines changed: 31 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -27,6 +27,7 @@ import 'package:makit/desktop/settings/sections/server_devices_section.dart';
2727
import 'package:makit/desktop/settings/server_config.dart';
2828
import 'package:makit/store/connection.dart';
2929
import 'package:shared_preferences/shared_preferences.dart';
30+
import 'package:makit/store/prefs/profile_scoped_prefs.dart';
3031

3132
const _socketPath = String.fromEnvironment('MAKIT_CONTROL_SOCK');
3233
const _timeout = Duration(seconds: 20);
@@ -92,7 +93,10 @@ void main() {
9293
controlClientProvider.overrideWithValue(client),
9394
desktopControllerProvider.overrideWithValue(controller),
9495
serverConfigProvider.overrideWith(
95-
(ref) => ServerConfigController(prefs, const ServerConfig()),
96+
(ref) => ServerConfigController(
97+
ProfileScopedPrefs.unscoped(prefs),
98+
const ServerConfig(),
99+
),
96100
),
97101
connectionProvider.overrideWithValue(MakitConnState()),
98102
],
@@ -139,7 +143,7 @@ void main() {
139143
);
140144
});
141145

142-
testWidgets('Endpoint bind-mode picker drives the unified ServerConfig', (
146+
testWidgets('Reachability picker drives the unified ServerConfig', (
143147
tester,
144148
) async {
145149
expect(
@@ -150,7 +154,10 @@ void main() {
150154

151155
SharedPreferences.setMockInitialValues({});
152156
final prefs = await SharedPreferences.getInstance();
153-
final config = ServerConfigController(prefs, const ServerConfig());
157+
final config = ServerConfigController(
158+
ProfileScopedPrefs.unscoped(prefs),
159+
const ServerConfig(),
160+
);
154161

155162
final client = ReconnectingControlClient(
156163
create: () => MakitControlClient(socketPath: _socketPath),
@@ -187,16 +194,32 @@ void main() {
187194
reason: 'lifecycle never showed a running daemon',
188195
);
189196

190-
// Ships defaulting to Auto (the new secure default) with no host field.
191-
expect(config.current.bindMode, ServerBindMode.auto);
197+
// The real daemon is up (the active-profile row reads "Running").
198+
await _pumpUntil(
199+
tester,
200+
find.text('Running'),
201+
reason: 'active-profile row never showed a running daemon',
202+
);
203+
204+
// Ships defaulting to "My devices" (the secure default), no host field.
205+
expect(config.current.reachability, Reachability.myDevices);
192206
expect(
193207
find.ancestor(of: find.text('Host'), matching: find.byType(TextField)),
194208
findsNothing,
195209
);
196210

197-
// Selecting Custom reveals a host field and persists the mode.
198-
await _scrollAndTap(tester, find.text('Custom'));
199-
expect(config.current.bindMode, ServerBindMode.custom);
211+
// Selecting "Just this Mac" pins loopback in serveArgs.
212+
await _scrollAndTap(tester, find.text('Just this Mac'));
213+
await tester.pumpAndSettle();
214+
expect(config.current.reachability, Reachability.thisMacOnly);
215+
expect(
216+
config.current.serveArgs(),
217+
containsAllInOrder(['--host', '127.0.0.1']),
218+
);
219+
220+
// The custom-host escape hatch lives under Diagnostics → Advanced.
221+
await _scrollAndTap(tester, find.text('Diagnostics'));
222+
await _scrollAndTap(tester, find.text('Advanced'));
200223
final host = find.ancestor(
201224
of: find.text('Host'),
202225
matching: find.byType(TextField),
@@ -212,9 +235,5 @@ void main() {
212235
config.current.serveArgs(),
213236
containsAllInOrder(['--host', '0.0.0.0']),
214237
);
215-
216-
// Switching to Loopback persists too (no daemon restart is triggered).
217-
await _scrollAndTap(tester, find.text('Loopback'));
218-
expect(config.current.bindMode, ServerBindMode.loopback);
219238
});
220239
}

app/integration_test/desktop/settings_repo_test.dart

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -38,6 +38,7 @@ import 'package:makit/desktop/settings/settings_window.dart';
3838
import 'package:makit/store/connection.dart';
3939
import 'package:makit/store/models.dart';
4040
import 'package:makit/store/store.dart';
41+
import 'package:makit/store/prefs/profile_scoped_prefs.dart';
4142
import 'package:makit/ui/home/repo_monogram.dart';
4243
import 'package:shared_preferences/shared_preferences.dart';
4344

@@ -107,7 +108,10 @@ Widget _app() => ProviderScope(
107108
overrides: [
108109
reposProvider.overrideWithValue(ReposState(_repos)),
109110
serverConfigProvider.overrideWith(
110-
(ref) => ServerConfigController(_prefs, const ServerConfig()),
111+
(ref) => ServerConfigController(
112+
ProfileScopedPrefs.unscoped(_prefs),
113+
const ServerConfig(),
114+
),
111115
),
112116
desktopControllerProvider.overrideWithValue(
113117
DesktopController(

app/lib/control/reconnecting_control_client.dart

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -97,9 +97,25 @@ class ReconnectingControlClient implements ControlClient {
9797
}
9898

9999
/// Disposes any live connection. Safe to call multiple times.
100+
///
101+
/// An in-flight connect is awaited first: `close()` only nulling `_current`
102+
/// would let a pending `_connectNew` complete *after* close and install a live
103+
/// socket into `_current` that nothing ever closes. This leaked the old
104+
/// profile's connection on a switch, which then kept polling under a torn-down
105+
/// runtime. Awaiting the pending connect lets it settle into `_current` (or
106+
/// fail and dispose itself), after which the single disposal below covers it.
100107
Future<void> close() async {
108+
final connecting = _connecting;
109+
if (connecting != null) {
110+
try {
111+
await connecting;
112+
} catch (_) {
113+
// A failed connect already disposed its client in _connectNew.
114+
}
115+
}
101116
final current = _current;
102117
_current = null;
118+
_connecting = null;
103119
if (current != null) await _safeDispose(current);
104120
}
105121

app/lib/control/reconnecting_control_client_test.dart

Lines changed: 40 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -158,5 +158,45 @@ void main() {
158158
expect(connected, [0, 1]);
159159
},
160160
);
161+
162+
test(
163+
'close() disposes a client whose connect was still in flight',
164+
() async {
165+
// Regression: close() used to only null `_current`, so a connect still
166+
// in flight would complete afterwards, install a live socket into
167+
// `_current`, and leak it — the old profile's runtime kept polling after
168+
// teardown.
169+
final localCreated = <_FakeClient>[];
170+
final localDisposed = <int>[];
171+
final gate = Completer<void>();
172+
var seq = 0;
173+
final client = ReconnectingControlClient(
174+
create: () {
175+
final c = _FakeClient(seq++);
176+
localCreated.add(c);
177+
return c;
178+
},
179+
connect: (_) async => gate.future, // stays in flight until released
180+
dispose: (c) async => localDisposed.add((c as _FakeClient).id),
181+
);
182+
183+
// Trigger connect but do not await the call yet.
184+
final pending = client.status();
185+
await Future<void>.delayed(Duration.zero);
186+
expect(localCreated, hasLength(1));
187+
188+
// Close while the connect is in flight, then let the connect complete.
189+
final closing = client.close();
190+
gate.complete();
191+
await closing;
192+
await pending.then((_) {}, onError: (_) {});
193+
194+
// The in-flight client must have been disposed, not retained.
195+
expect(localDisposed, contains(0));
196+
// And it is not reused: the next call connects a fresh client.
197+
await client.status();
198+
expect(localCreated, hasLength(2));
199+
},
200+
);
161201
});
162202
}

app/lib/desktop/chat/groups/groups_controller.dart

Lines changed: 8 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -17,7 +17,8 @@ import 'dart:convert';
1717

1818
import 'package:flutter/foundation.dart';
1919
import 'package:flutter_riverpod/legacy.dart';
20-
import 'package:shared_preferences/shared_preferences.dart';
20+
21+
import '../../../store/prefs/profile_scoped_prefs.dart';
2122

2223
import '../panes/split_node.dart';
2324
import '../panes/workspace_controller.dart';
@@ -200,10 +201,10 @@ class GroupsController extends StateNotifier<GroupsState> {
200201

201202
/// Builds a controller from persisted state, migrating the SPEC-28 single
202203
/// workspace when this is the first run with groups.
203-
static GroupsController load(SharedPreferences prefs) =>
204+
static GroupsController load(ScopedPrefs prefs) =>
204205
GroupsController(prefs, decode(prefs));
205206

206-
final SharedPreferences? _prefs;
207+
final ScopedPrefs? _prefs;
207208

208209
/// The group with [id], or null.
209210
Group? groupById(String id) {
@@ -506,7 +507,7 @@ class GroupsController extends StateNotifier<GroupsState> {
506507
/// Decodes persisted state, migrating the SPEC-28 single workspace on first
507508
/// run and falling back to [GroupsState.fresh] for anything unusable.
508509
@visibleForTesting
509-
static GroupsState decode(SharedPreferences prefs) {
510+
static GroupsState decode(ScopedPrefs prefs) {
510511
final raw = prefs.getString(kGroupsPrefsKey);
511512
if (raw == null || raw.isEmpty) return _migrateLegacy(prefs);
512513
Object? decoded;
@@ -579,7 +580,7 @@ class GroupsController extends StateNotifier<GroupsState> {
579580
/// First run with groups: fold the SPEC-28 single workspace into one board so
580581
/// nobody loses their layout. Empty tabs are carried over verbatim (decision
581582
/// 21) and bound tabs become the board's membership.
582-
static GroupsState _migrateLegacy(SharedPreferences prefs) {
583+
static GroupsState _migrateLegacy(ScopedPrefs prefs) {
583584
final legacyRaw = prefs.getString(kWorkspacePrefsKey);
584585
if (legacyRaw == null || legacyRaw.isEmpty) return GroupsState.fresh();
585586
final tree = WorkspaceController.decodeWorkspace(legacyRaw);
@@ -711,7 +712,8 @@ class GroupsController extends StateNotifier<GroupsState> {
711712
}
712713

713714
/// The groups layer. Defaults to a non-persisting controller; `runDesktopApp`
714-
/// overrides it with a [SharedPreferences]-backed one, and tests may too.
715+
/// overrides it with a profile-scoped [ScopedPrefs]-backed one, and tests may
716+
/// too.
715717
final groupsControllerProvider =
716718
StateNotifierProvider<GroupsController, GroupsState>(
717719
(ref) => GroupsController.ephemeral(),

0 commit comments

Comments
 (0)