fix(sync): defer LocalFile folder pick commit to settings Save (#9075) (#9085)

* fix(sync): defer LocalFile folder pick commit to settings Save (#9075)

* test(sync): pin Android SAF pick route through form value (#9075)

electron/electronAPI.d.ts

@@ -59,16 +59,39 @@ export interface ElectronAPI {
   checkDirExists(args: { relativePath?: string }): Promise<true | Error>;
 
   /**
-   * Opens the native folder picker for the sync folder. Resolves to:
-   * - `string`: the canonicalized, persisted folder path on success
+   * Opens the native folder picker for the sync folder. Prepare-only (#9075):
+   * the pick is held main-side as a pending candidate and does NOT become the
+   * live sync target until `commitPickedDirectory()` (settings Save).
+   * Resolves to:
+   * - `string`: the canonicalized candidate folder path (display only)
    * - `undefined`: the user cancelled the picker
-   * - `Error`: the pick succeeded but main could not canonicalize/persist it
-   *   (e.g. the folder was deleted between pick and commit, EACCES, or the
-   *   folder lives inside the app's private dir). Nothing is persisted in
+   * - `Error`: the pick succeeded but main could not canonicalize/validate it
+   *   (e.g. the folder was deleted right after picking, EACCES, or the
+   *   folder lives inside the app's private dir). No candidate is stored in
    *   this case; the renderer must treat it as a failure, not a picked path.
    */
   pickDirectory(): Promise<string | Error | undefined>;
 
+  /**
+   * Persists the pending picked folder (see `pickDirectory`) as the live sync
+   * target. Call from settings Save only. Resolves to:
+   * - `{ path, isChanged }`: committed; `isChanged` is false when the user
+   *   re-picked the folder that was already configured (callers must skip
+   *   target-change invalidation in that case)
+   * - `null`: no pending candidate (routine save without a pick) — a no-op
+   * - `Error`: validation/persistence failed (e.g. folder deleted between
+   *   pick and Save). The candidate is kept so a retry fails loudly instead
+   *   of silently saving without the folder change.
+   */
+  commitPickedDirectory(): Promise<{ path: string; isChanged: boolean } | null | Error>;
+
+  /**
+   * Drops the pending picked folder without touching the live sync target.
+   * Call when the settings UI closes without a save. No-op if nothing is
+   * pending.
+   */
+  discardPickedDirectory(): Promise<void>;
+
   /**
    * Returns the main-owned sync folder path for display, or null if not yet
    * configured. The renderer must not pass this value back to file-sync IPCs

electron/local-file-sync.test.cjs

@@ -24,6 +24,16 @@ let handlers = {};
 let userDataDir;
 let externalDir;
 let nextDialogResult = { canceled: true, filePaths: [] };
+let winStub;
+
+const makeWinStub = () => ({
+  webContents: {
+    _handlers: {},
+    on(evt, fn) {
+      this._handlers[evt] = fn;
+    },
+  },
+});
 
 const installMocks = () => {
   Module._load = function patchedLoad(request, parent, isMain) {
@@ -40,7 +50,7 @@ const installMocks = () => {
       return { log: () => {}, error: () => {} };
     }
     if (/[/\\]main-window$/.test(request)) {
-      return { getWin: () => ({}) };
+      return { getWin: () => winStub };
     }
     return originalModuleLoad.call(this, request, parent, isMain);
   };
@@ -59,17 +69,20 @@ const load = () => {
 };
 
 const configureSyncFolder = async (folder) => {
-  // Drive the real PICK_DIRECTORY path so the test exercises the same
-  // canonicalize-and-persist code production uses.
+  // Drive the real pick → commit path so the test exercises the same
+  // prepare-then-persist code production uses (#9075).
   nextDialogResult = { canceled: false, filePaths: [folder] };
-  const result = await handlers['PICK_DIRECTORY']({});
-  if (result instanceof Error) throw result;
-  return result;
+  const picked = await handlers['PICK_DIRECTORY']({});
+  if (picked instanceof Error) throw picked;
+  const committed = await handlers['COMMIT_PICKED_DIRECTORY']({});
+  if (committed instanceof Error) throw committed;
+  return committed.path;
 };
 
 test.beforeEach(() => {
   userDataDir = fs.realpathSync.native(fs.mkdtempSync(path.join(os.tmpdir(), 'sp-ud-')));
   externalDir = fs.realpathSync.native(fs.mkdtempSync(path.join(os.tmpdir(), 'sp-ext-')));
+  winStub = makeWinStub();
   installMocks();
   load();
 });
@@ -263,30 +276,189 @@ test('GET_SYNC_FOLDER_PATH returns null when unconfigured, then the configured p
   assert.equal(await handlers['GET_SYNC_FOLDER_PATH']({}), externalDir);
 });
 
-test('PICK_DIRECTORY persists the selection main-side', async () => {
+test('PICK_DIRECTORY is prepare-only: nothing goes live until commit (#9075)', async () => {
   nextDialogResult = { canceled: false, filePaths: [externalDir] };
   const returned = await handlers['PICK_DIRECTORY']({});
-  assert.equal(returned, externalDir);
+  assert.equal(returned, externalDir, 'returns the candidate for display');
   assert.equal(
     await handlers['GET_SYNC_FOLDER_PATH']({}),
-    externalDir,
-    'main-side store is updated; renderer does not have to echo back',
+    null,
+    'live target untouched by the pick',
   );
+
+  const committed = await handlers['COMMIT_PICKED_DIRECTORY']({});
+  assert.deepEqual(committed, { path: externalDir, isChanged: true });
+  assert.equal(await handlers['GET_SYNC_FOLDER_PATH']({}), externalDir);
 });
 
-test('PICK_DIRECTORY surfaces a safe error when persistence fails', async () => {
+test('a sync between pick and commit still resolves against the OLD folder (#9075)', async () => {
+  const oldDir = externalDir;
+  const newDir = fs.realpathSync.native(
+    fs.mkdtempSync(path.join(os.tmpdir(), 'sp-new-')),
+  );
+  try {
+    await configureSyncFolder(oldDir);
+    nextDialogResult = { canceled: false, filePaths: [newDir] };
+    await handlers['PICK_DIRECTORY']({});
+
+    const result = await handlers['FILE_SYNC_SAVE'](
+      {},
+      { relativePath: 'main.json', dataStr: '{"ok":1}', localRev: null },
+    );
+    assert.ok(!(result instanceof Error));
+    assert.ok(fs.existsSync(path.join(oldDir, 'main.json')), 'written to old folder');
+    assert.equal(fs.existsSync(path.join(newDir, 'main.json')), false);
+  } finally {
+    fs.rmSync(newDir, { recursive: true, force: true });
+  }
+});
+
+test('DISCARD_PICKED_DIRECTORY abandons the pick; the old target stays live (#9075)', async () => {
+  const oldDir = externalDir;
+  const newDir = fs.realpathSync.native(
+    fs.mkdtempSync(path.join(os.tmpdir(), 'sp-new-')),
+  );
+  try {
+    await configureSyncFolder(oldDir);
+    nextDialogResult = { canceled: false, filePaths: [newDir] };
+    await handlers['PICK_DIRECTORY']({});
+    await handlers['DISCARD_PICKED_DIRECTORY']({});
+
+    assert.equal(await handlers['GET_SYNC_FOLDER_PATH']({}), oldDir);
+    assert.equal(
+      await handlers['COMMIT_PICKED_DIRECTORY']({}),
+      null,
+      'a later save has nothing to commit',
+    );
+    assert.equal(await handlers['GET_SYNC_FOLDER_PATH']({}), oldDir);
+  } finally {
+    fs.rmSync(newDir, { recursive: true, force: true });
+  }
+});
+
+test('COMMIT_PICKED_DIRECTORY without a pick is a null no-op (routine save)', async () => {
+  await configureSyncFolder(externalDir);
+  assert.equal(await handlers['COMMIT_PICKED_DIRECTORY']({}), null);
+  assert.equal(await handlers['GET_SYNC_FOLDER_PATH']({}), externalDir);
+});
+
+test('picking twice keeps only the last candidate', async () => {
+  const otherDir = fs.realpathSync.native(
+    fs.mkdtempSync(path.join(os.tmpdir(), 'sp-other-')),
+  );
+  try {
+    nextDialogResult = { canceled: false, filePaths: [otherDir] };
+    await handlers['PICK_DIRECTORY']({});
+    nextDialogResult = { canceled: false, filePaths: [externalDir] };
+    await handlers['PICK_DIRECTORY']({});
+
+    const committed = await handlers['COMMIT_PICKED_DIRECTORY']({});
+    assert.equal(committed.path, externalDir);
+  } finally {
+    fs.rmSync(otherDir, { recursive: true, force: true });
+  }
+});
+
+test('re-picking the configured folder commits with isChanged:false', async () => {
+  // The renderer gates its target-change invalidation (cursor wipe) on
+  // isChanged — a same-folder re-pick must not report a move.
+  await configureSyncFolder(externalDir);
+  nextDialogResult = { canceled: false, filePaths: [externalDir] };
+  await handlers['PICK_DIRECTORY']({});
+  const committed = await handlers['COMMIT_PICKED_DIRECTORY']({});
+  assert.deepEqual(committed, { path: externalDir, isChanged: false });
+});
+
+test('PICK_DIRECTORY surfaces a safe error when validation fails', async () => {
   // Picker returns a path that no longer exists on disk → realpath throws
   // → PICK_DIRECTORY must return a distinct Error, not undefined, so the
-  // renderer doesn't confuse persist-failure with user-cancel.
+  // renderer doesn't confuse validation failure with user-cancel.
   const gone = fs.mkdtempSync(path.join(os.tmpdir(), 'sp-gone-'));
   fs.rmSync(gone, { recursive: true, force: true });
   nextDialogResult = { canceled: false, filePaths: [gone] };
   const result = await handlers['PICK_DIRECTORY']({});
   assert.ok(result instanceof Error);
-  // Cache must not be poisoned with a non-existent path.
+  // No candidate stored: a later save must not commit the bad path.
+  assert.equal(await handlers['COMMIT_PICKED_DIRECTORY']({}), null);
   assert.equal(await handlers['GET_SYNC_FOLDER_PATH']({}), null);
 });
 
+test('COMMIT_PICKED_DIRECTORY errors when the folder vanished between pick and save', async () => {
+  const oldDir = externalDir;
+  const doomed = fs.realpathSync.native(
+    fs.mkdtempSync(path.join(os.tmpdir(), 'sp-doomed-')),
+  );
+  await configureSyncFolder(oldDir);
+  nextDialogResult = { canceled: false, filePaths: [doomed] };
+  await handlers['PICK_DIRECTORY']({});
+  fs.rmSync(doomed, { recursive: true, force: true });
+
+  const result = await handlers['COMMIT_PICKED_DIRECTORY']({});
+  assert.ok(result instanceof Error);
+  assert.equal(
+    await handlers['GET_SYNC_FOLDER_PATH']({}),
+    oldDir,
+    'live target not poisoned by the failed commit',
+  );
+  // The candidate is kept so a retried save fails loudly instead of
+  // silently saving without the folder change.
+  const retry = await handlers['COMMIT_PICKED_DIRECTORY']({});
+  assert.ok(retry instanceof Error);
+});
+
+test('a discard while the native picker is open prevents arming an orphaned candidate (#9075)', async () => {
+  // closeAllDialogs()/reload can tear down the settings dialog (running its
+  // discard) while the native folder picker is still open. The resolving
+  // pick must NOT arm a candidate nobody owns — a later unrelated save would
+  // silently commit it.
+  await configureSyncFolder(externalDir);
+  let resolveDialog;
+  nextDialogResult = new Promise((resolve) => (resolveDialog = resolve));
+  const newDir = fs.realpathSync.native(
+    fs.mkdtempSync(path.join(os.tmpdir(), 'sp-new-')),
+  );
+  try {
+    const pickPromise = handlers['PICK_DIRECTORY']({});
+    await handlers['DISCARD_PICKED_DIRECTORY']({});
+    resolveDialog({ canceled: false, filePaths: [newDir] });
+
+    assert.equal(await pickPromise, undefined, 'disowned pick reads as cancel');
+    assert.equal(await handlers['COMMIT_PICKED_DIRECTORY']({}), null);
+    assert.equal(await handlers['GET_SYNC_FOLDER_PATH']({}), externalDir);
+  } finally {
+    fs.rmSync(newDir, { recursive: true, force: true });
+  }
+});
+
+test('a renderer reload clears the pending pick — no surprise commit on a later save (#9075)', async () => {
+  const newDir = fs.realpathSync.native(
+    fs.mkdtempSync(path.join(os.tmpdir(), 'sp-new-')),
+  );
+  try {
+    await configureSyncFolder(externalDir);
+    nextDialogResult = { canceled: false, filePaths: [newDir] };
+    await handlers['PICK_DIRECTORY']({});
+
+    // The dialog's renderer-side discard hook dies with the page on reload;
+    // main must drop the candidate itself.
+    winStub.webContents._handlers['did-navigate']();
+
+    assert.equal(await handlers['COMMIT_PICKED_DIRECTORY']({}), null);
+    assert.equal(await handlers['GET_SYNC_FOLDER_PATH']({}), externalDir);
+  } finally {
+    fs.rmSync(newDir, { recursive: true, force: true });
+  }
+});
+
+test('PICK_DIRECTORY rejects a folder inside userData without storing a candidate', async () => {
+  const inside = path.join(userDataDir, 'sneaky');
+  fs.mkdirSync(inside);
+  nextDialogResult = { canceled: false, filePaths: [inside] };
+  const result = await handlers['PICK_DIRECTORY']({});
+  assert.ok(result instanceof Error);
+  assert.equal(await handlers['COMMIT_PICKED_DIRECTORY']({}), null);
+});
+
 test('PICK_DIRECTORY returns undefined on user-cancel (distinct from error)', async () => {
   nextDialogResult = { canceled: true, filePaths: [] };
   const result = await handlers['PICK_DIRECTORY']({});

electron/local-file-sync.ts

@@ -34,6 +34,41 @@ const getAppPrivateDir = (): string => app.getPath('userData');
 let _cachedSyncFolder: string | null | undefined = undefined;
 let _loadPromise: Promise<string | null> | null = null;
 
+// Folder picked but not yet committed via settings Save (#9075). Memory-only
+// on purpose: a crash or close-without-save must leave the live sync target
+// untouched. Main-owned so the renderer never round-trips an absolute path
+// (#8228) — commit/discard reference this slot, they don't carry a path.
+let _pendingSyncFolder: string | null = null;
+
+// Bumped by every discard (IPC or renderer-death reset below). A pick
+// snapshots it before opening the native dialog and refuses to arm the slot
+// if it moved meanwhile: a discard during an open picker means the owning
+// settings UI is gone (closeAllDialogs(), reload, non-modal WM quirks), and
+// arming anyway would orphan a candidate that a later unrelated Save would
+// silently commit.
+let _discardGeneration = 0;
+
+const _discardPendingSyncFolder = (): void => {
+  _pendingSyncFolder = null;
+  _discardGeneration++;
+};
+
+// A renderer reload/crash destroys the settings dialog without running its
+// discard hook, which would leave a picked-but-never-saved folder armed to
+// commit on an unrelated Save much later. Drop the candidate whenever the
+// renderer navigates away or dies (in-app Angular routing is same-document
+// and does not fire did-navigate). Installed lazily on first pick — the main
+// window is guaranteed to exist there.
+let _isPendingResetHookInstalled = false;
+const _installPendingResetHook = (win: ReturnType<typeof getWin>): void => {
+  if (_isPendingResetHookInstalled || !win?.webContents?.on) {
+    return;
+  }
+  win.webContents.on('did-navigate', _discardPendingSyncFolder);
+  win.webContents.on('render-process-gone', _discardPendingSyncFolder);
+  _isPendingResetHookInstalled = true;
+};
+
 const _loadSyncFolderFromDisk = async (): Promise<string | null> => {
   const all = await loadSimpleStoreAll();
   const raw = all[SimpleStoreKey.SYNC_FOLDER_PATH];
@@ -287,7 +322,10 @@ export const initLocalFileSyncAdapter = (): void => {
 
   ipcMain.handle(IPC.PICK_DIRECTORY, async (): Promise<string | Error | undefined> => {
     try {
-      const { canceled, filePaths } = (await dialog.showOpenDialog(getWin(), {
+      const win = getWin();
+      _installPendingResetHook(win);
+      const discardGenAtOpen = _discardGeneration;
+      const { canceled, filePaths } = (await dialog.showOpenDialog(win, {
         title: 'Select sync folder',
         buttonLabel: 'Select Folder',
         properties: [
@@ -300,19 +338,69 @@ export const initLocalFileSyncAdapter = (): void => {
       if (canceled || !filePaths[0]) {
         return undefined;
       }
-      // Persist main-side BEFORE returning the display string. If
-      // canonicalization or persistence fails (deleted between pick and
-      // commit, EACCES on userData, etc.), surface a safe error rather than
-      // a silent undefined — otherwise the renderer cannot distinguish
-      // failure from user-cancel and the user is left wondering why their
-      // pick didn't take.
-      return await setSyncFolderPath(filePaths[0]);
+      if (_discardGeneration !== discardGenAtOpen) {
+        // The settings UI that owned this pick was torn down (its discard
+        // already ran) while the native dialog was open — nobody is left to
+        // commit or discard the result. Treat as cancel instead of arming an
+        // ownerless candidate.
+        return undefined;
+      }
+      // Prepare-only (#9075): validate NOW so a bad pick errors at pick time,
+      // but do NOT persist or swap the live root — a sync firing between pick
+      // and settings Save must still hit the old folder, and Cancel must be
+      // able to abandon the pick. COMMIT_PICKED_DIRECTORY makes it live.
+      // Surfacing validation failures as a safe error (not undefined) keeps
+      // them distinguishable from user-cancel.
+      const canonical = realpathSync.native(filePaths[0]);
+      assertPathOutside(getAppPrivateDir(), canonical);
+      _pendingSyncFolder = canonical;
+      return canonical;
     } catch (e) {
-      error('PICK_DIRECTORY failed to persist sync folder', getSafeErrorMeta(e));
+      error('PICK_DIRECTORY failed to validate picked folder', getSafeErrorMeta(e));
       return createSafeIpcError(IPC.PICK_DIRECTORY, e);
     }
   });
 
+  ipcMain.handle(
+    IPC.COMMIT_PICKED_DIRECTORY,
+    async (): Promise<{ path: string; isChanged: boolean } | null | Error> => {
+      try {
+        // Capture the slot before any await: a DISCARD or PICK landing while
+        // `prev` loads must not turn this commit into a null-deref or swap
+        // the path under it mid-flight.
+        const pending = _pendingSyncFolder;
+        if (pending === null) {
+          // Nothing picked this session (or already committed/discarded) —
+          // a routine settings Save without a pick. Not an error.
+          return null;
+        }
+        const prev = await getSyncFolderPath();
+        // Re-canonicalize + re-validate at commit time: the folder can be
+        // deleted or symlink-swapped between pick and Save. On failure the
+        // pending slot is kept so a retry stays loud (errors again) instead
+        // of silently saving without the folder change; the user re-picks
+        // or cancels.
+        const committed = await setSyncFolderPath(pending);
+        // Only clear the slot if it still holds what we committed — a newer
+        // pick that raced in must survive for its own commit.
+        if (_pendingSyncFolder === pending) {
+          _pendingSyncFolder = null;
+        }
+        // isChanged lets the renderer fire its target-change invalidation
+        // only on a real move — re-picking the same folder must not wipe
+        // per-target sync state (see notifyProviderTargetChanged docs).
+        return { path: committed, isChanged: committed !== prev };
+      } catch (e) {
+        error('COMMIT_PICKED_DIRECTORY failed to persist', getSafeErrorMeta(e));
+        return createSafeIpcError(IPC.COMMIT_PICKED_DIRECTORY, e);
+      }
+    },
+  );
+
+  ipcMain.handle(IPC.DISCARD_PICKED_DIRECTORY, async (): Promise<void> => {
+    _discardPendingSyncFolder();
+  });
+
   ipcMain.handle(IPC.GET_SYNC_FOLDER_PATH, async (): Promise<string | null> => {
     return getSyncFolderPath();
   });

electron/preload.ts

@@ -60,6 +60,11 @@ const ea: ElectronAPI = {
   checkDirExists: (args) => _invoke('CHECK_DIR_EXISTS', args) as Promise<true | Error>,
 
   pickDirectory: () => _invoke('PICK_DIRECTORY') as Promise<string | Error | undefined>,
+  commitPickedDirectory: () =>
+    _invoke('COMMIT_PICKED_DIRECTORY') as Promise<
+      { path: string; isChanged: boolean } | null | Error
+    >,
+  discardPickedDirectory: () => _invoke('DISCARD_PICKED_DIRECTORY') as Promise<void>,
   getSyncFolderPath: () => _invoke('GET_SYNC_FOLDER_PATH') as Promise<string | null>,
 
   showOpenDialog: (options: {

electron/shared-with-frontend/ipc-events.const.ts

@@ -47,6 +47,8 @@ export enum IPC {
 
   CHECK_DIR_EXISTS = 'CHECK_DIR_EXISTS',
   PICK_DIRECTORY = 'PICK_DIRECTORY',
+  COMMIT_PICKED_DIRECTORY = 'COMMIT_PICKED_DIRECTORY',
+  DISCARD_PICKED_DIRECTORY = 'DISCARD_PICKED_DIRECTORY',
   GET_SYNC_FOLDER_PATH = 'GET_SYNC_FOLDER_PATH',
   IMAGE_PICK_AND_IMPORT = 'IMAGE_PICK_AND_IMPORT',
   IMAGE_CACHE_GET_DATA_URL = 'IMAGE_CACHE_GET_DATA_URL',