plans/better-state-machine.md
Proposed implementation plan, revised after review. Three decisions from that review are incorporated here:
notify-error), not a value-equal state-identity change. The
resurface-nonce machinery in registry.ts is never ported to the new
manager.StateMachineDefinition/CommandExecutor interfaces are introduced in this
plan; see "What is generic and what stays domain" for the rationale.Unlike the previous draft, changing the transition graph is in scope: the graph gains events for restore-to-message and diff-file selection. The user-visible flow is otherwise preserved, with one deliberate exception documented in section 2 (selection is no longer restored after an app switch).
The current implementation has the right central idea: callers send events through a controller and render from the controller snapshot. The remaining problems are around, not inside, that machine.
React components
| events | singleton presentation atoms
v v
module-global registry -------> Jotai store
| controller per app
v
state machine ChatMessage / MessagesList
| commands | revertVersion / restoreToMessage
v v
renderer command adapter -- IPC --> main-process version handlers
^ |
+---- infers effects from caches ---+
Six issues follow from this shape:
ChatMessage.tsx and MessagesList.tsx mutate
the repository through useVersions (revertVersion,
restoreToMessage) without going through the machine. The machine
serializes its own mutations (isMutatingState, mutationInFlight) but
cannot see these, so an out-of-band revert can race a machine checkout, and
after one succeeds the machine's checkedOutVersionId is stale truth.
selectedVersionReturnBranchAtom exists only to smuggle machine state
(the origin branch) out to this bypass path.diffVersionIdForState() from its own session.registry.ts owns controllers, runtime initialization, recovery entries,
and listeners in module globals. This hides lifecycle and makes test
isolation depend on reset functions.recovery-required + OPEN returns a fresh, value-equal state object
purely so subscribers re-notify and the recovery toast re-surfaces
(transition.ts). The registry then needs a resurfaceNonce counter plus
a memoized equality dance (~60 lines) to separate that signal from the
noise the hack itself created — and correctness depends on the
controller's reference-inequality notify, so a future value-equality
"optimization" would silently kill re-surfacing.useEffect diffing
selectedAppIdAtom; a lifecycle-critical policy depends on render timing.VersionPreviewProvider (one per renderer application root)
|
+-- VersionPreviewManager (domain facade)
| +-- KeyedControllerHost<VersionPreviewController>
| | +-- one controller per appId
| | +-- app-scoped snapshots/subscriptions/disposal
| +-- version-preview recovery policy and entries
| +-- app-switch return policy (subscribes to the Jotai store directly)
| +-- renderer command adapter (the ONLY caller of version-mutation IPC)
|
+-- TanStack Query: IPC-backed server/main-process data
+-- apps, versions, chats, settings
main-process IPC handlers
+-- perform the mutation under the app-scoped lock
+-- report the effects that actually occurred
The ownership rules are explicit:
selectedAppIdAtom remains; it is
app-shell state, not version state.)The previous draft extracted a generic kernel
(StateMachineDefinition<State, Event, Command, Input>,
CommandExecutor<Command, Event, Context>, a generic controller). That is
dropped, for two reasons:
Input parameter
had no client at all (the initial state is the constant CLOSED_STATE).controller.ts). Only the origin-resolution read uses
latest-wins epochs. A generic controller would need per-command
concurrency policy — a design decision that should wait for a second data
point.What is extracted instead is the lifecycle machinery, which is genuinely domain-independent today:
interface KeyedController {
getSnapshot(): unknown;
subscribe(listener: () => void): () => void;
dispose(): void;
}
class KeyedControllerHost<K, C extends KeyedController> {
constructor(create: (key: K) => C);
ensure(key: K): C;
get(key: K): C | undefined;
keys(): K[];
subscribeKey(key: K, listener: () => void): () => void;
subscribeAny(listener: () => void): () => void;
disposeKey(key: K): void;
dispose(): void;
}
The host owns:
useKeyedController(host, key, selectSnapshot) adapter over
useSyncExternalStore with stable snapshot identity.The host must not own:
VersionPreviewController keeps its current domain-specific shape (pure
transition() plus command execution with per-command concurrency rules).
The second, third, and fourth machines have since arrived (PRs #3968 plan
handoff, #3969 app run, #3970 connection flow) and confirmed this split:
their concurrency models all differ, while their lifecycle plumbing is
identical. plans/machine-followup.md owns the shared-kernel scope
(KeyedControllerHost, snapshot store, transition types, test kit) and the
migration of the other machines; whichever plan lands first creates
src/state_machines/, and the other consumes it unchanged.
Today ChatMessage.tsx calls restoreToMessage (IPC
restoreToMessageVersion) with targetBranchName read from
selectedVersionReturnBranchAtom, and MessagesList.tsx calls
revertVersion for undo/retry flows. Both bypass the machine.
Extend the machine instead:
// New/changed events
| { type: "RESTORE"; appId: number; versionId: string }
| {
type: "RESTORE_TO_MESSAGE";
appId: number;
chatId: number;
messageId: number;
restoreCodebase: boolean;
}
// New/changed commands
| { type: "restore"; appId: number; versionId: string; targetBranch: string | null }
| {
type: "restore-to-message";
appId: number;
chatId: number;
messageId: number;
restoreCodebase: boolean;
targetBranch: string | null;
}
Transition rules:
closed + RESTORE/RESTORE_TO_MESSAGE → restoring with a fresh
session. originBranch is null, so the command's targetBranch is
null and the handler restores onto the live branch (today's
no-targetBranchName behavior).browsing behaves like closed (no checkout is owned yet).previewing + either event → restoring with
targetBranch = session.originBranch, exactly as the atom-passing path
behaves today. Success lands on the origin branch, so the session ends and
the state returns to closed.checking-out / restoring / returning / recovery-required ignore
both events. The UI disables restore affordances from the machine snapshot
(see below), so an ignored event is a race lost, not a UX dead end.This gives real mutual exclusion: the transition matrix cannot start a
restore while another mutation is in flight, which the current
isAnyVersionMutationPending counter only approximates.
ChatMessage.tsx sends RESTORE_TO_MESSAGE via useVersionPreview(appId)
instead of calling the mutation. Navigation to the newChatId created by
the restore becomes a post-effect applied by the command adapter from the
IPC result (section 3), so the component no longer orchestrates it.MessagesList.tsx undo/retry flows send RESTORE events.useVersions drops revertVersion and restoreToMessage mutations and
keeps read queries. isAnyVersionMutationPending is replaced by a
derivation from the machine snapshot (isMutatingState), exposed through
the version-preview hooks. Note this also deletes the
onMutate/mutationAppId staleness workaround in useVersions.ts — the
session owns its appId, so the completion cannot attribute effects to the
wrong app.selectedVersionReturnBranchAtom and the VersionPane.tsx effect
that mirrors machine state into it. No getReturnBranch() accessor is
added to the manager — with all mutations routed through the machine,
nothing outside it needs the origin branch.ipc.version.checkoutVersion, revertVersion, and
restoreToMessageVersion.previewing state survives an out-of-band revert,
because out-of-band reverts no longer exist.Add presentation fields to the session instead of creating an app-keyed Jotai map:
export interface PreviewSession {
appId: number;
originBranch: string | null;
targetVersionId: string | null;
checkedOutVersionId: string | null;
exitIntent: ExitIntent;
/** Presentation only. Never used to decide Git transitions. */
selectedDiffFile: { versionId: string; path: string } | null;
/** Presentation only; closing it never returns or checks out a branch. */
isDiffVisible: boolean;
}
Presentation events mutate these fields without emitting repository commands:
| { type: "SELECT_DIFF_FILE"; file: { versionId: string; path: string } | null }
| { type: "CLOSE_VERSION_DIFF" }
| { type: "VIEW_VERSION_DIFF"; appId: number; versionId: string; file: ... }
Rules:
SELECT_DIFF_FILE is honored while a diff is visible and ignored elsewhere.
It emits no commands, ever.viewing-diff state,
so chat stays mounted while the Code panel shows the requested commit.CLOSE_VERSION_DIFF only clears diff presentation; CLOSE remains the
repository-workflow exit that returns an owned historical checkout.selectedDiffFile.targetVersionId /
diffVersionIdForState()); consumers read it from the snapshot instead of
selectedVersionIdAtom. No second copy exists to fall out of sync.Delete selectedVersionIdAtom and selectedVersionDiffFileAtom from
src/atoms/appAtoms.ts (selectedVersionReturnBranchAtom is deleted by
section 1).
Selection now dies with the session. Switching from app A to app B drains app A's session (background return to the origin branch) and closes it; coming back to app A shows live state with no restored selection. The previous draft's criterion — "returning to app A restores its presentation selection" — is dropped on purpose: it restored a selection pointing at a version that was no longer checked out, for a pane that was no longer open. If persist-across-close selection is ever wanted, it must be validated against the machine snapshot at read time; do not resurrect a second store with an independent lifetime.
Update these consumers to read selection from useVersionPreview(appId)
snapshots (via small selector helpers, e.g. selectedDiffFile(state),
selectedVersionId(state)), passing an explicit appId in reusable leaf
components:
src/components/chat/ChatMessage.tsxsrc/components/chat/ModifiedFilesCard.tsxsrc/components/chat/VersionPane.tsxsrc/components/preview_panel/CodeView.tsxsrc/components/preview_panel/PreviewToolbar.tsxsrc/components/preview_panel/CommitMenu.tsxsrc/components/preview_panel/VersionDiffView.tsxIf profiling shows selection-only consumers re-rendering too often on machine
transitions, add a selector variant of the hook
(useVersionPreviewSelector(appId, selector, isEqual)); do not solve it by
moving state back out of the machine.
SELECT_DIFF_FILE produces a new state object for the owning app only;
subscribers of other apps are not notified.Add a result schema to src/ipc/types/version.ts and use it for all
version-preview mutations:
const VersionCommandResultSchema = z.object({
repositoryOutcome: z.enum(["target-applied", "unchanged"]),
notification: z
.object({
kind: z.enum(["success", "warning"]),
message: z.string(),
})
.nullable(),
runtimeAction: z.enum(["none", "restart"]),
affectedChatId: z.number().nullable(),
/** Set only by restore-to-message when a new chat was created. */
createdChatId: z.number().nullable(),
});
The contract is capability-oriented: runtimeAction: "restart" tells the
renderer what to do without exposing Neon- or cloud-specific logic across the
IPC boundary. repositoryOutcome lets the machine distinguish a completed
restore from fork-only or warning/no-op restore-to-message results; it must not
discard an owned preview session unless main confirms the target was applied.
Do not infer "return to the live branch" from gitRef === "main". A
repository's live branch need not be named main, and a commit/ref could
collide with that convention.
Change the checkout input to a discriminated union:
type CheckoutVersionInput =
| { purpose: "preview"; appId: number; versionId: string }
| { purpose: "return"; appId: number; branch: string };
The handler chooses database/environment behavior from intent rather than
string comparison. Audit and migrate every checkoutVersion caller in the
same change so the contract cannot be used ambiguously. (After section 1
there should be exactly one caller: the command adapter.)
Within the app-scoped lock, the main-process handlers accumulate effects from operations that actually completed:
runtimeAction to restart when the mutation changed the runtime
environment or when the active runtime requires a restart after
synchronization.createdChatId when restore-to-message created a new chat.If lower-level helpers such as revertCodebaseToVersion perform environment
changes, extend their internal return value so the IPC handler receives facts
instead of re-querying or predicting them.
Update src/version_preview/commands.ts so each command:
applyVersionCommandResult(result) helper (runtimeAction,
affectedChatId refresh, createdChatId navigation, notification);finally.Remove correctness decisions based on:
hasDbSnapshot supplied by the UI/controller;selectedChatIdAtom read after restore completion.Because the main process now decides runtimeAction, the renderer no longer
needs hasDbSnapshot at all: delete the field from events, session, and
commands once the metadata contract lands. The domain model shrinks with the
inference it existed to feed.
The adapter may use getQueryData only for optional display enrichment.
Missing or stale cache data must not change mutation correctness.
A failed IPC mutation remains a command failure and drives the machine's failure event. A renderer post-effect failure — a query invalidation, chat navigation, or runtime refresh failure after Git already succeeded — must not make the controller believe the Git mutation failed.
Implement this boundary explicitly:
hasDbSnapshot no longer appears in the domain model.main origin branch performs return semantics
correctly.IpcMainInvokeEvent types.Add under src/state_machines/:
keyed_host.ts — KeyedControllerHost as specified above;react.ts — useKeyedController, a useSyncExternalStore adapter with
stable snapshot identity;Do not add generic transition/command/executor types, and do not introduce a global registry of hosts. Providers construct the hosts they need.
"Re-surface the recovery toast" is a one-shot effect, and the machine already
has the right primitive for effects: commands executed by the runtime
(notify-error is the precedent). Replace the identity-change signaling with
commands:
// state.ts — new commands
| { type: "notify-recovery"; appId: number; error: PreviewError }
| { type: "dismiss-recovery"; appId: number }
Transition changes:
returning + RETURN_FAILED → recovery-required, emitting
notify-recovery.recovery-required + OPEN → same state, same reference, emitting
notify-recovery. The { ...state } clone is deleted; states change
reference only when they change value.recovery-required + RETRY_RETURN → returning, emitting
dismiss-recovery alongside the return command.returning + RETURN_SUCCEEDED needs no dismiss: the retry path already
dismissed on RETRY_RETURN. If a future path can leave recovery without
passing through RETRY_RETURN, it must emit dismiss-recovery.The command adapter implements both with the toast layer exactly as
notify-error does today: toast.error with a stable per-app id,
duration: Infinity, and a Retry action that sends RETRY_RETURN through
the manager; toast.dismiss for the counterpart. One non-transition dismiss
path remains: manager.disposeApp(appId) (app deletion) must dismiss any
outstanding recovery toast for that app before disposing the controller.
This deletes, rather than ports, the compensating machinery in
registry.ts: recoveryNonceByAppId, the resurfaceNonce entry field,
sameRecoveryEntries(), and the recoveryCache/lastRecoveryEntries
identity dance. Recovery entries — still exposed for UI that lists stuck
apps — become a plain derived view over controller snapshots: filter for
recovery-required, memoized by the snapshots themselves, which are now
reliably reference-stable.
Replace src/version_preview/registry.ts with a thin
src/version_preview/manager.ts facade:
class VersionPreviewManager {
constructor(deps: {
host: KeyedControllerHost<number, VersionPreviewController>;
store: JotaiStore; // for selectedAppIdAtom subscription
});
getSnapshot(appId: number): PreviewState;
send(appId: number, event: PreviewEvent): void;
subscribeApp(appId: number, listener: () => void): () => void;
getRecoveryEntries(): VersionPreviewRecoveryEntry[];
subscribeRecovery(listener: () => void): () => void;
disposeApp(appId: number): void;
dispose(): void;
}
The generic host owns the controller map, per-app subscriptions, and disposal. The facade owns only domain policy:
store.sub(selectedAppIdAtom, ...)) at construction time and released in
dispose(). Draining the previous app's session must not depend on a React
effect firing; the provider owns the manager's lifetime, not its policy
timing.disposeApp().There is no getReturnBranch(): section 1 removed its only consumer.
Keep controller creation lazy per app, but make both manager creation and
ownership explicit. dispose() disposes the host, unsubscribes from the
store, and clears recovery subscriptions.
Create VersionPreviewProvider near the renderer application root. It:
Both React bridges are gone: the manager owns app-switch draining, and recovery toasts are issued and dismissed by machine commands through the adapter. The provider is pure context plumbing.
Hooks:
useVersionPreview(appId) wraps useKeyedController and returns the
snapshot plus send.useVersionPreviewManager() supports imperative operations such as app
deletion (manager.disposeApp(appId)).useVersionPreviewRecovery() subscribes only to recovery entries.Use useSyncExternalStore with stable snapshot identities. A change to app A
must not rerender hooks subscribed to app B.
Delete:
resetVersionPreviewForTests and tests that depend on it;Tests construct a fake command adapter, host, and manager, then dispose them normally. This makes lifecycle behavior production-shaped and allows multiple isolated managers in one test process.
src/state_machines/ has no imports from version preview, IPC, TanStack
Query, Jotai, or notification code.selectedAppIdAtom, and is tested without rendering.Tests are added alongside each phase rather than deferred until the end.
The existing totality/invariant suite in transition.test.ts extends to the
new events:
RESTORE and RESTORE_TO_MESSAGE from closed, browsing, and
previewing, including targetBranch null versus origin;SELECT_DIFF_FILE honored only while a version diff is visible, never emitting
commands, cleared on version change; read-only viewing-diff presentation
stays outside Version History and CLOSE_VERSION_DIFF never emits Git work;target-applied from
unchanged, retaining preview ownership for fork-only and warning outcomes;RETURN_FAILED emits notify-recovery; OPEN in recovery-required
returns the same state reference and emits notify-recovery;
RETRY_RETURN emits dismiss-recovery with the return command;notify-recovery/dismiss-recovery reach the adapter's toast functions
on failure, re-open, retry, and disposeApp;Expand src/version_preview/commands.test.ts with table-driven cases for
every command and both success and failure paths:
createdChatId
navigation);Add focused handler or integration tests for:
main origin branch;runtimeAction matching actual environment changes;affectedChatId for message-linked and commit-linked
versions, and createdChatId for restore-to-message;Use the IPC integration harness if real handler wiring, sqlite state, or fake runtime routes are required; keep pure result aggregation tests at the unit level.
Add e2e-tests/version_preview_lifecycle.spec.ts with one focused scenario:
This validates the real Electron shell, manager lifecycle, background Git return, and machine-owned presentation together. Keep mid-operation ordering and recovery edge cases deterministic in manager/controller tests rather than adding production delays to the E2E.
Before running the E2E, rebuild the application:
npm run build
npx playwright test e2e-tests/version_preview_lifecycle.spec.ts
User-visible behavior is preserved. The one graph change in this phase is
deliberate: replace the recovery re-surface identity hack with
notify-recovery/dismiss-recovery commands before building the manager,
so the nonce machinery is deleted rather than migrated and manager.test.ts
never encodes it.
KeyedControllerHost and useKeyedController with their tests.state.ts/transition.ts
and implement them in the command adapter; delete the { ...state }
re-notify branch.VersionPreviewManager; app-switch subscribes to the store directly.registry.ts.RESTORE (from closed/browsing) and
RESTORE_TO_MESSAGE; extend the restore command payloads.ChatMessage.tsx and MessagesList.tsx to machine events; remove
the mutations from useVersions; derive pending state from the snapshot.selectedVersionReturnBranchAtom and the VersionPane mirror
effect.selectedDiffFile and SELECT_DIFF_FILE; migrate all presentation
consumers to snapshot reads.selectedVersionIdAtom and selectedVersionDiffFileAtom.applyVersionCommandResult; delete
hasDbSnapshot from the domain model.Likely new files:
src/state_machines/keyed_host.tssrc/state_machines/keyed_host.test.tssrc/state_machines/react.tssrc/state_machines/react.test.tsxsrc/version_preview/manager.tssrc/version_preview/manager.test.tssrc/version_preview/VersionPreviewProvider.tsxe2e-tests/version_preview_lifecycle.spec.tsLikely modified files:
src/version_preview/state.ts — session presentation fields, new events,
command payload changes, hasDbSnapshot removalsrc/version_preview/transition.ts and transition.test.tssrc/version_preview/controller.ts — minor; keeps its domain shapesrc/version_preview/commands.ts and commands.test.tssrc/hooks/useVersionPreview.tssrc/hooks/useVersions.ts — mutations removed, reads keptsrc/atoms/appAtoms.ts — three atoms deletedsrc/ipc/types/version.tssrc/ipc/handlers/version_handlers.tssrc/components/chat/ChatMessage.tsx, MessagesList.tsx,
ModifiedFilesCard.tsx, VersionPane.tsxsrc/components/preview_panel/CodeView.tsx, PreviewToolbar.tsx,
CommitMenu.tsx, VersionDiffView.tsxLikely removed file:
src/version_preview/registry.ts, after all imports have migratedThe exact handler test file should follow the existing IPC test organization discovered during implementation rather than creating a parallel harness.
Unlike the previous draft, the graph changes (new restore paths, selection
events). The totality and invariant tests in transition.test.ts are the
safety net; extend them in the same commit as each graph change, and keep
Phase 0 characterization tests for the flows being rerouted.
RESTORE/RESTORE_TO_MESSAGE from closed sends targetBranch: null,
relying on the handler's existing restore-onto-live-branch behavior when
targetBranchName is omitted. Verify that behavior with a handler test
before migrating callers.
Selection is intentionally not restored after an app switch. Flag this in the PR description and validate in Phase 5 manual inspection; if product wants persistence, it must be re-added as machine-validated state, not a parallel store.
Command-driven toasts trade the self-healing reconcile-a-list bridge for
explicit dismiss paths. The dismiss set is small and closed — RETRY_RETURN
and disposeApp — but each must be tested, and any future transition that
exits recovery-required by a new route must emit dismiss-recovery. The
transition-test invariant ("no value-equal state with a new reference")
guards the other direction: nobody can quietly reintroduce identity
signaling.
The manager needs the query client and Jotai store, so the provider mounts below those providers and above all version-preview consumers. Construct once with a ref or stable memo, and test unmount/remount explicitly.
The reusable surface is deliberately only the keyed host and React adapter. If a second machine appears mid-implementation, resist merging its needs into this plan; extract shared controller mechanics as a follow-up informed by both machines.
useSyncExternalStore loops or broad rerendersSnapshots must retain identity until the subscribed app changes. Keep separate app and recovery listener sets, and add notification-count assertions to manager tests. Selection now lives in the snapshot, so watch for selection-only consumers over-rendering; add the selector hook variant if profiling demands it.
A discriminated input intentionally breaks every ambiguous caller at
type-check time. Migrate all callers in one phase and run npm run ts before
considering the phase complete. Removing the useVersions mutations gives
the same compile-time guarantee for the single-writer migration.
Git may succeed before a restart, navigation, or cache refresh fails. Preserve repository truth in the machine and surface the secondary problem separately. Add a test specifically for this split.
Returning to the origin branch is asynchronous. Poll Git branch and worktree state rather than using fixed sleeps, and keep the test to one lifecycle scenario to limit flakiness and runtime.
Run the narrowest tests during each phase, followed by the complete pre-commit checks:
npm test -- src/state_machines/keyed_host.test.ts
npm test -- src/state_machines/react.test.tsx
npm test -- src/version_preview/manager.test.ts
npm test -- src/version_preview/commands.test.ts
npm test -- src/version_preview/controller.test.ts
npm test -- src/version_preview/transition.test.ts
npm test -- src/components/chat/VersionPane.test.tsx
npm run fmt
npm run lint
npm run ts
npm run build
npx playwright test e2e-tests/version_preview_lifecycle.spec.ts
Adjust targeted component/handler paths to match the final test placement.
Inspect git status after formatter and lint fixes to ensure no unrelated
files changed.
hasDbSnapshot is gone from the domain model.