Consistent settings behaviour#133
Merged
Clydingus merged 6 commits intoMay 8, 2026
Merged
Conversation
Settings whose changes need a restart used to live in three drifting hand-written lists (useEngineRespawn for process-class, useSessionInit for session/live, plus EngineTab.hasChangesRequiringRestart for the modal trigger). Adding engine_backend to the session list was missed, so toggling it mid-session saved without a reset and dropped back to the pause menu. SETTING_CLASSES in types/settings.ts is now the single source of truth, with helpers in utils/settingsClassifier consuming it. Top-level keys cover whole subtrees; dot-paths split when siblings differ (debug_overlays.action_logging is live; siblings are UI-only).
Toggling a process-class setting mid-stream raced: useEngineRespawn called stopServer() fire-and-forget then transitioned to LOADING immediately. The warm flow read stale isServerRunning=true from React state, picked attachToRunningStandalone, and threw "Server exited before becoming ready" while the doomed server finished dying. Awaiting stopServer lets the IPC "server exited" event propagate to isServerRunning=false before LOADING fires, so the warm flow correctly picks bootStandalone and starts a fresh server with the new env vars.
In offline mode uv can't reach the index to verify the lockfile and fails with "No solution found when resolving dependencies" — even though the venv is already correctly populated from the previous online sync. Setting UV_NO_SYNC=1 (which also implies --frozen) inside getOfflineEnv skips both the resolve and sync passes on uv run, so the server just exec's python in the existing venv. Online mode is untouched: uv run still auto-syncs when pyproject changes.
The connectionLost flag was only cleared in MAIN_MENU, so once set it stuck through LOADING and back into STREAMING. Hit visibly when a process-class respawn (useEngineRespawn) fires its disconnect from outside the lifecycle reducer — the brief failed-connection render inside STREAMING latched connectionLost true, and entering LOADING didn't clear it. Clearing on LOADING entry is symmetric with the existing clearEngineErrorOnLoadingEntry: any LOADING transition is "starting fresh", so prior overlays should reset. Also fixes the click-reconnect recovery path, which previously kept the overlay up until the user backed out to main menu. (Follow-up: the overlay still flashes for a frame during a respawn because the disconnect lands in a STREAMING render before LOADING transitions in. To suppress it entirely, the reducer needs to know the respawn is intentional — separate change.)
Toggling a process-class setting mid-stream briefly flashed the
"Connection Lost" overlay. The session-class reconnect path already
suppresses the overlay via `intentionalReconnectInProgress`, but
process-class respawns are driven by useEngineRespawn (outside the
reducer) and never set that flag.
Rather than adding a parallel `intentionalRespawnInProgress` boolean
plus parallel `currentProcessSig` / `lastAppliedProcess` payload
fields plus parallel detection branches, collapse all of it through a
single discriminator:
- New `RestartSignatures = { session, process }` bundle in the
classifier; `getRestartSignatures(settings)` builds both at once.
- `useSessionInit` returns `lastApplied: RestartSignatures | null`
(one piece of state instead of two).
- Payload carries `currentSignatures` + `lastAppliedSignatures`.
- Reducer state replaces `intentionalReconnectInProgress` with
`intentionalRestart: 'reconnect' | 'respawn' | null`. A new
`computeIntentionalRestart()` picks the strongest applicable
intent (process beats session) and returns null when nothing's
pending.
- Side-effect dispatch keys off the discriminator: `'reconnect'`
fires the existing effect chain, `'respawn'` is silent because
useEngineRespawn owns those side effects. Suppression checks
become `intentionalRestart !== null`.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Toggling the engine backend mid-session did not reset the server; instead, it went to the pause menu. I realised that the logic for choosing to restart or reset the server was all over the place, which meant that this bug would keep reoccurring. To fix this, I refactored the associated code to institute one source of truth. Afterwards, while testing it, I discovered that there were several bugs with offline mode.
Single source of truth for restart classification
Every
Settingsfield now has an associated class (none/live/session/process) declared once insrc/types/settings.ts. All downstream code responsible for controlling the server now consults that map, ensuring consistency of behaviour.The classifier supports both top-level keys (covering whole subtrees) and dot-paths (when siblings disagree, e.g.
debug_overlays.action_loggingislivewhile its UI-only siblings default tonone).Lifecycle bugs surfaced while testing the refactor
A handful of pre-existing races and gaps showed up once mid-session restart was actually being exercised:
Diagnostic clarity
The
check-engine-statusdependency-validation step now surfaces uv's actual stderr/stdout when it fails, instead of silently returning false. Saves a round of guessing the next time a venv ends up out of sync.