Persist only the checkbox selection of uncommitted changes

apps/desktop/src/lib/selection/uncommitted.ts

@@ -29,6 +29,19 @@ type UncommittedState = {
 	hunkSelection: EntityState<HunkSelection, string>;
 };
 
+/**
+ * The parts of this slice that must not survive a restart.
+ *
+ * Both are refetched from the worktree on startup, so a persisted copy is only ever the previous
+ * session's, rendered for the frames before the fresh query replaces it. `hunkSelection` is
+ * deliberately absent: the checkboxes are the state worth keeping, and `update()` rebuilds them
+ * against whatever assignments arrive.
+ */
+export const UNCOMMITTED_PERSIST_BLACKLIST = [
+	"treeChanges",
+	"hunkAssignments",
+] as const satisfies readonly (keyof UncommittedState)[];
+
 /**
  * State representing uncommitted changes.
  *

apps/desktop/src/lib/selection/uncommittedService.svelte.ts

@@ -9,6 +9,7 @@ import {
 import { showToast } from "$lib/notifications/toasts";
 import { compositeKey, partialKey, type HunkSelection } from "$lib/selection/entityAdapters";
 import {
+	UNCOMMITTED_PERSIST_BLACKLIST,
 	uncommittedSelectors,
 	uncommittedSlice,
 	type CheckboxStatus,
@@ -59,7 +60,9 @@ export class UncommittedService {
 		private diffService: DiffService,
 	) {
 		this.dispatch = clientState.dispatch;
-		const getSlice = clientState.injectPersistedSlice(uncommittedSlice);
+		const getSlice = clientState.injectPersistedSlice(uncommittedSlice, [
+			...UNCOMMITTED_PERSIST_BLACKLIST,
+		]);
 
 		$effect(() => {
 			this.state = getSlice() ?? uncommittedSlice.getInitialState();

apps/desktop/src/lib/state/clientState.svelte.ts

@@ -1,4 +1,5 @@
 import { createBackendApi, type BackendApi } from "$lib/state/backendApi";
+import { persistConfigFor } from "$lib/state/persistConfig";
 import { uiStateSlice } from "$lib/state/uiState.svelte";
 import { InjectionToken } from "@gitbutler/core/context";
 import { mergeUnlisten } from "@gitbutler/ui/utils/mergeUnlisten";
@@ -65,11 +66,21 @@ export class ClientState {
 		});
 	}
 
-	injectPersistedSlice<S>(slice: Slice<S>): () => S | undefined {
+	/**
+	 * Persist `slice` across restarts, restoring it before the first render.
+	 *
+	 * `blacklist` names parts of the slice that should not survive. State that a query refetches
+	 * on startup belongs there: persisting it means the previous session's copy is rendered
+	 * first and replaced moments later, which reads as a flash of stale data.
+	 */
+	injectPersistedSlice<S extends object>(
+		slice: Slice<S>,
+		blacklist?: Extract<keyof S, string>[],
+	): () => S | undefined {
 		this.reducer.inject(
 			{
 				reducerPath: slice.reducerPath,
-				reducer: persistReducer({ key: slice.reducerPath, storage }, slice.reducer),
+				reducer: persistReducer(persistConfigFor(slice.reducerPath, blacklist), slice.reducer),
 			},
 			{ overrideExisting: false },
 		);

apps/desktop/src/lib/state/persistBlacklist.test.ts

@@ -0,0 +1,97 @@
+import {
+	UNCOMMITTED_PERSIST_BLACKLIST,
+	uncommittedActions,
+	uncommittedSlice,
+} from "$lib/selection/uncommitted";
+import { persistConfigFor } from "$lib/state/persistConfig";
+import { configureStore } from "@reduxjs/toolkit";
+import { persistReducer } from "redux-persist";
+import persistStore from "redux-persist/lib/persistStore";
+import { describe, expect, test } from "vitest";
+import type { Storage } from "redux-persist";
+
+/** Records what redux-persist writes, so tests can assert on the persisted shape itself. */
+function recordingStorage(seed?: Record<string, string>): Storage & {
+	written(): Record<string, string>;
+} {
+	const items: Record<string, string> = { ...seed };
+	return {
+		getItem: async (key: string) => items[key] ?? null,
+		setItem: async (key: string, value: string) => {
+			items[key] = value;
+		},
+		removeItem: async (key: string) => {
+			delete items[key];
+		},
+		written: () => items,
+	};
+}
+
+type UncommittedState = ReturnType<typeof uncommittedSlice.getInitialState>;
+
+const KEY = uncommittedSlice.reducerPath;
+const BLACKLIST = [...UNCOMMITTED_PERSIST_BLACKLIST];
+
+/** A change and its assignment, enough to populate every part of the slice. */
+const CHANGE = { path: "a.txt", status: { type: "Modification" } } as never;
+const ASSIGNMENT = {
+	id: "assignment-1",
+	path: "a.txt",
+	pathBytes: "a.txt",
+	stackId: null,
+	hunkHeader: null,
+	lineNumsAdded: null,
+	lineNumsRemoved: null,
+} as never;
+
+/**
+ * Run the real slice through a real persist cycle under the config the app uses, with only
+ * storage swapped out, and report what reached storage and what the store holds.
+ */
+async function persistCycle(seed?: Record<string, string>) {
+	const storage = recordingStorage(seed);
+	const store = configureStore({
+		reducer: persistReducer(
+			{ ...persistConfigFor<UncommittedState>(KEY, BLACKLIST), storage },
+			uncommittedSlice.reducer,
+		),
+		middleware: (getDefault) => getDefault({ serializableCheck: false }),
+	});
+	await new Promise<void>((resolve) => persistStore(store, undefined, () => resolve()));
+	// What the app would render on startup: rehydrated, but before the worktree query lands.
+	const rehydrated = store.getState();
+	store.dispatch(uncommittedActions.update({ assignments: [ASSIGNMENT], changes: [CHANGE] }));
+	// redux-persist writes on a timeout, so let the queued write run.
+	await new Promise((resolve) => setTimeout(resolve, 50));
+
+	const raw = storage.written()[`persist:${KEY}`];
+	return {
+		persisted: raw ? Object.keys(JSON.parse(raw)).filter((k) => k !== "_persist") : [],
+		rehydrated,
+	};
+}
+
+describe("uncommitted slice persistence", () => {
+	// Types already stop a name that is not part of the slice; this stops the opposite mistake.
+	test("the checkbox state is not blacklisted", () => {
+		expect(BLACKLIST).not.toContain("hunkSelection");
+	});
+
+	test("only the checkbox state is written", async () => {
+		const { persisted } = await persistCycle();
+		expect(persisted).toEqual(["hunkSelection"]);
+	});
+
+	// Anything an earlier build already wrote has to be dropped on the way in as well, or the
+	// first launch after upgrading rehydrates a stale file list and flashes it once more.
+	test("state left by an earlier build is not rehydrated", async () => {
+		const stale = JSON.stringify({
+			treeChanges: JSON.stringify({ ids: ["gone.txt"], entities: { "gone.txt": CHANGE } }),
+			hunkAssignments: JSON.stringify({ ids: [], entities: {} }),
+			hunkSelection: JSON.stringify({ ids: [], entities: {} }),
+			_persist: JSON.stringify({ version: -1, rehydrated: true }),
+		});
+		const { rehydrated } = await persistCycle({ [`persist:${KEY}`]: stale });
+		expect(rehydrated.treeChanges.ids).toEqual([]);
+	});
+});

apps/desktop/src/lib/state/persistConfig.ts

@@ -0,0 +1,31 @@
+import autoMergeLevel1 from "redux-persist/lib/stateReconciler/autoMergeLevel1";
+import storage from "redux-persist/lib/storage";
+import type { PersistConfig } from "redux-persist";
+
+/**
+ * Persist configuration for a slice, keeping the `blacklist`ed keys out of storage.
+ *
+ * `blacklist` on its own only stops those keys being written. Whatever an earlier build already
+ * wrote is still read back and merged on the next launch, so the reconciler drops them on the
+ * way in as well; without that, the first launch after upgrading still restores stale state.
+ */
+export function persistConfigFor<S extends object>(
+	key: string,
+	blacklist?: Extract<keyof S, string>[],
+): PersistConfig<S> {
+	if (!blacklist || blacklist.length === 0) {
+		return { key, storage };
+	}
+	return {
+		key,
+		storage,
+		blacklist,
+		stateReconciler: (inbound: S, original: S, reduced: S, config: PersistConfig<S>) => {
+			const kept = { ...inbound };
+			for (const name of blacklist) {
+				delete kept[name];
+			}
+			return autoMergeLevel1(kept, original, reduced, config);
+		},
+	};
+}