diff --git a/apps/desktop/src/lib/selection/uncommitted.ts b/apps/desktop/src/lib/selection/uncommitted.ts index 9b61279a75b..914ac354722 100644 --- a/apps/desktop/src/lib/selection/uncommitted.ts +++ b/apps/desktop/src/lib/selection/uncommitted.ts @@ -29,6 +29,19 @@ type UncommittedState = { hunkSelection: EntityState; }; +/** + * 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. * diff --git a/apps/desktop/src/lib/selection/uncommittedService.svelte.ts b/apps/desktop/src/lib/selection/uncommittedService.svelte.ts index c16a8c31180..c55243f4b22 100644 --- a/apps/desktop/src/lib/selection/uncommittedService.svelte.ts +++ b/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(); diff --git a/apps/desktop/src/lib/state/clientState.svelte.ts b/apps/desktop/src/lib/state/clientState.svelte.ts index 62f4cc349f5..71019bd2719 100644 --- a/apps/desktop/src/lib/state/clientState.svelte.ts +++ b/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(slice: Slice): () => 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( + slice: Slice, + blacklist?: Extract[], + ): () => 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 }, ); diff --git a/apps/desktop/src/lib/state/persistBlacklist.test.ts b/apps/desktop/src/lib/state/persistBlacklist.test.ts new file mode 100644 index 00000000000..9412225cb55 --- /dev/null +++ b/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): Storage & { + written(): Record; +} { + const items: Record = { ...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; + +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) { + const storage = recordingStorage(seed); + const store = configureStore({ + reducer: persistReducer( + { ...persistConfigFor(KEY, BLACKLIST), storage }, + uncommittedSlice.reducer, + ), + middleware: (getDefault) => getDefault({ serializableCheck: false }), + }); + await new Promise((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([]); + }); +}); diff --git a/apps/desktop/src/lib/state/persistConfig.ts b/apps/desktop/src/lib/state/persistConfig.ts new file mode 100644 index 00000000000..babc41dcc7c --- /dev/null +++ b/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( + key: string, + blacklist?: Extract[], +): PersistConfig { + if (!blacklist || blacklist.length === 0) { + return { key, storage }; + } + return { + key, + storage, + blacklist, + stateReconciler: (inbound: S, original: S, reduced: S, config: PersistConfig) => { + const kept = { ...inbound }; + for (const name of blacklist) { + delete kept[name]; + } + return autoMergeLevel1(kept, original, reduced, config); + }, + }; +}