From 07039bd6c8862997accfaee585d2efbd09f80ea9 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Mon, 19 Oct 2026 04:02:45 +0000 Subject: [PATCH 1/4] chore: record implementation plan From d3601314c4efec706febeac460a5b7e90cbbf2bf Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Mon, 19 Oct 2026 04:23:39 +0000 Subject: [PATCH 2/4] refactor(settings): enable direct settings saves Co-authored-by: ScriptedAlchemy <25274700+ScriptedAlchemy@users.noreply.github.com> --- .../settings/EffectiveConfigTable.tsx | 29 +- .../settings/SettingsEditorController.tsx | 46 +- ...eviewPanel.tsx => SettingsEditorPanel.tsx} | 219 ++----- .../workspaces/settings/SettingsInspector.tsx | 37 +- .../settings/SettingsPage.dom.test.tsx | 243 +++---- .../src/workspaces/settings/SettingsPage.tsx | 44 +- .../workspaces/settings/SettingsValues.tsx | 23 +- .../src/workspaces/settings/settingsChrome.ts | 3 +- .../settings/settingsEditorMachine.test.ts | 595 ++++-------------- .../settings/settingsEditorMachine.ts | 371 +++-------- .../src/workspaces/settings/settingsRows.ts | 8 +- 11 files changed, 472 insertions(+), 1146 deletions(-) rename dashboard/src/workspaces/settings/{SettingsReviewPanel.tsx => SettingsEditorPanel.tsx} (66%) diff --git a/dashboard/src/workspaces/settings/EffectiveConfigTable.tsx b/dashboard/src/workspaces/settings/EffectiveConfigTable.tsx index 270bada841..d29b719c82 100644 --- a/dashboard/src/workspaces/settings/EffectiveConfigTable.tsx +++ b/dashboard/src/workspaces/settings/EffectiveConfigTable.tsx @@ -3,8 +3,8 @@ * one row per served scalar, grouped under sticky section rows. * * Hover and focus INSPECT a row: the inspector shows its exact evidence and - * nothing on the row changes. Click, Enter, or Space SELECT it: the review - * state opens directly under it. Escape closes the review, then clears the + * nothing on the row changes. Click, Enter, or Space SELECT it: the editor + * opens directly under it. Escape closes the editor, then clears the * inspection. Arrow keys rove between rows so a reader never has to Tab through * fifty keys to reach the one they want. * @@ -30,7 +30,7 @@ import type { SettingsEditorHandle } from './SettingsEditorController.tsx'; import type { SettingsEditorState } from './settingsEditorMachine.ts'; import type { WritableScopes } from './settingsGates.ts'; import type { ConfigSection } from './settingsModel.ts'; -import { SettingsReviewPanel } from './SettingsReviewPanel.tsx'; +import { SettingsEditorPanel } from './SettingsEditorPanel.tsx'; import { bindingFor, draftValue, @@ -67,15 +67,12 @@ const COLUMNS = '@min-[41rem]:grid-cols-[minmax(9rem,1.3fr)_minmax(8rem,1.2fr)_6.5rem_minmax(9.5rem,1.2fr)_4.5rem]'; const COLUMN_GAP = 'gap-x-2'; -/** The machine states in which a frozen review, and its controls, are on screen. */ -const REVIEW_BEARING: ReadonlySet = new Set([ - 'reviewing', - 'confirmed', +/** States whose save controls can resolve back to the editor. */ +const SAVE_BEARING: ReadonlySet = new Set([ 'submitting', 'conflicted', 'authority_withdrawn', 'submit_failed', - 'review_superseded', ]); export function EffectiveConfigTable({ @@ -117,17 +114,17 @@ export function EffectiveConfigTable({ // hide the verdict the reader is waiting for. const pinned = editor.state.status === 'submitting'; - // Focus returns to the edited row when a frozen review resolves and the - // control the reader was on, Apply, Retry, Load current values, unmounts - // with the review stage. Only when focus was actually lost to the document: - // a keystroke in another scope's input also ends a staged review, and that + // Focus returns to the edited row when a save resolves and the control the + // reader was on, Retry or Load current values, unmounts. Only when focus was + // actually lost to the document: + // a keystroke in another scope's input also ends a pending save, and that // input must keep its focus. const status = editor.state.status; const previousStatus = useRef(status); useEffect(() => { - const wasStaged = REVIEW_BEARING.has(previousStatus.current); + const wasSaving = SAVE_BEARING.has(previousStatus.current); previousStatus.current = status; - if (!wasStaged || status !== 'editing' || selectedKey === null) return; + if (!wasSaving || status !== 'editing' || selectedKey === null) return; const active = document.activeElement; const container = scrollRef.current; if (active === null || active === document.body || !container?.contains(active)) { @@ -151,7 +148,7 @@ export function EffectiveConfigTable({ } return; } - // Rove only from a row; a key pressed inside the review panel's inputs + // Rove only from a row; a key pressed inside the editor's inputs // belongs to the input. if (!(event.target instanceof HTMLElement) || event.target.getAttribute('role') !== 'row') { return; @@ -236,7 +233,7 @@ export function EffectiveConfigTable({ {selected ? (
- void; /** Take the authority's value back for one bound field, leaving other edits alone. */ readonly revert: (binding: SettingsBinding) => void; - /** Freeze this scope's change against the held revision. */ - readonly review: (scope: SettingsScope) => void; - readonly setConfirmed: (confirmed: boolean) => void; - readonly apply: () => void; - readonly dismiss: () => void; + /** Save this scope's current draft against its held revision. */ + readonly save: (scope: SettingsScope) => void; + /** Retry the same immutable write with its original idempotency key. */ + readonly retry: () => void; /** Discard every draft and take the authority's current values. */ readonly reload: () => void; } @@ -83,17 +78,16 @@ export function useSettingsEditor({ }, }); - // The request is derived from the submitting state, so it cannot be built - // from a draft the machine has not checked. The ref keeps one entry into - // `submitting` to one PATCH; leaving the state releases it for a retry. + // One transition into `submitting` can issue one request. Leaving that state + // releases the guard for a retry with the same idempotency key. const inFlight = useRef(null); useEffect(() => { if (state.status !== 'submitting') { inFlight.current = null; return; } - if (inFlight.current === state.review.reviewId) return; - inFlight.current = state.review.reviewId; + if (inFlight.current === state.submission.idempotencyKey) return; + inFlight.current = state.submission.idempotencyKey; // Project and ordinary user writes use the current project gateway reading. // The ProfileSessions worker preference is profile-global, so its request // never inherits selected-project writability. @@ -126,27 +120,23 @@ export function useSettingsEditor({ [state], ); - const review = useCallback((scope: SettingsScope) => { + const save = useCallback((scope: SettingsScope) => { dispatch({ - type: 'review_requested', + type: 'save_requested', scope, idempotencyKey: mintBrowserIdempotencyKey('dashboard-settings'), }); }, []); - const setConfirmed = useCallback((confirmed: boolean) => { - dispatch({ type: 'confirmation_set', confirmed }); - }, []); - const apply = useCallback(() => dispatch({ type: 'submit_started' }), []); - const dismiss = useCallback(() => dispatch({ type: 'review_dismissed' }), []); + const retry = useCallback(() => dispatch({ type: 'retry_requested' }), []); const reload = useCallback(() => { dispatch({ type: 'reloaded_from_authority' }); onApplied(); }, [onApplied]); return useMemo( - () => ({ state, edit, revert, review, setConfirmed, apply, dismiss, reload }), - [state, edit, revert, review, setConfirmed, apply, dismiss, reload], + () => ({ state, edit, revert, save, retry, reload }), + [state, edit, revert, save, retry, reload], ); } diff --git a/dashboard/src/workspaces/settings/SettingsReviewPanel.tsx b/dashboard/src/workspaces/settings/SettingsEditorPanel.tsx similarity index 66% rename from dashboard/src/workspaces/settings/SettingsReviewPanel.tsx rename to dashboard/src/workspaces/settings/SettingsEditorPanel.tsx index a96da1ec11..1f5298e162 100644 --- a/dashboard/src/workspaces/settings/SettingsReviewPanel.tsx +++ b/dashboard/src/workspaces/settings/SettingsEditorPanel.tsx @@ -1,12 +1,9 @@ /** - * The review state for one selected key, drawn inline under its row. + * The editor for one selected key, drawn inline under its row. * - * It renders one state of the editor machine and reports intent back. It never - * decides whether a change may be applied, the machine does, so a stage that - * cannot submit simply offers no way to try. What it always shows is the - * ladder the brief names: the effective value stays authoritative; a proposal - * is a proposal until the write authority validates it, persists it against - * the held revision, reports its apply requirement, and is read back. + * It renders the current draft and reports edits and Save intent to the + * controller. The effective value remains authoritative until the write is + * applied and read back. */ import { Lock, X } from 'lucide-react'; @@ -18,17 +15,12 @@ import { secondarySettingsButtonClass, settingsButtonClass } from './settingsChr import type { SettingsEditorHandle } from './SettingsEditorController.tsx'; import { settingsApplied, - settingsConfirmationHeld, settingsRejection, - settingsReviewOf, settingsScopePlan, + settingsSubmissionOf, type SettingsEditorState, } from './settingsEditorMachine.ts'; -import { - settingsRevisionId, - type SettingsScope, - type SettingsValidationError, -} from './settingsModel.ts'; +import type { SettingsScope, SettingsValidationError } from './settingsModel.ts'; import { SettingsRowEditor } from './SettingsRowEditor.tsx'; import { applyRequirement, @@ -43,10 +35,10 @@ import { ApplyRequirementText, ORIGIN_WORD, ValueCell, - reviewStatusWord, + saveStatusWord, } from './SettingsValues.tsx'; -export function SettingsReviewPanel({ +export function SettingsEditorPanel({ row, capability, editor, @@ -60,20 +52,20 @@ export function SettingsReviewPanel({ editor: SettingsEditorHandle; workerStatus: CodeIndexWorkerStatusV1 | null; query: string; - /** False while a write is in flight: the panel stays until the verdict lands. */ + /** False while a write is in flight so its outcome stays visible. */ closable: boolean; onClose: () => void; }) { return (

- {capability.kind === 'writable' ? 'Review' : 'Inspect'} + {capability.kind === 'writable' ? 'Edit' : 'Inspect'} {row.key} @@ -83,7 +75,7 @@ export function SettingsReviewPanel({ type="button" onClick={onClose} disabled={!closable} - aria-label="Close review" + aria-label="Close editor" className="td-hit group -my-2 -mr-2 shrink-0 disabled:cursor-wait disabled:opacity-50" > @@ -133,9 +125,8 @@ function PanelBody({

- Read-only. GET /api/settings reports this - key as effective configuration, and no settings PATCH route addresses it. Nothing on - this surface can propose a change to it. + Not editable here. Change this value in + the configuration source that owns it.

); @@ -165,7 +156,7 @@ function PanelBody({ Read-only · {capability.reason}

- + ); case 'writable': @@ -187,34 +178,31 @@ function PanelBody({ } /** - * A frozen review that outlived its gate: the scope moved, or the authority - * withdrew, after a change was staged. The locked branch cannot apply it, but - * it must still be able to let go of it, otherwise `review_dismissed` is - * unreachable and the machine sits in a verdict no control can leave. + * A save that outlived its gate remains visible, but a locked scope can only + * discard the old draft and reload current values. */ -function HeldReviewUnderLock({ +function HeldSubmissionUnderLock({ scope, editor, }: { scope: SettingsScope; editor: SettingsEditorHandle; }) { - const review = settingsReviewOf(editor.state); - if (review === null || review.scope !== scope) return null; + const submission = settingsSubmissionOf(editor.state); + if (submission === null || submission.scope !== scope) return null; return ( -
+
- A {scopeNoun(scope)} review is {reviewStatusWord(editor.state.status)} against revision{' '} - {review.expectedRevisionId}; this scope can no longer - apply it. + This {scopeNoun(scope)} save is {saveStatusWord(editor.state.status)}. This setting is now + read-only here; reload current values before editing it elsewhere.
); @@ -241,21 +229,20 @@ function WritableBody({ if (state.status === 'editor_unavailable') { return (

- Settings editing requires project configuration values and configuration_revision_id - from GET /api/settings, plus user settings and configuration_revision_id from the same - authority. The response omitted at least one required field. + Settings cannot be edited until the current configuration and its revision are available. + Reload settings and try again.

); } const scope = binding.scope; - const review = settingsReviewOf(state); - const scopeReview = review?.scope === scope ? review : null; + const submission = settingsSubmissionOf(state); + const scopeSubmission = submission?.scope === scope ? submission : null; const plan = settingsScopePlan(state, scope); const rejection = settingsRejection(state); const scopeErrors = rejection?.scope === scope ? rejection.errors : []; // The input wears whichever refusal names its field: a verdict the editor is // resting on first, else the live plan's, so `aria-invalid` is true the - // moment the value would be refused rather than only after a review attempt. + // moment the value would be refused rather than only after Save. // (An edit clears a resting refusal in the machine, so the two never // describe different drafts.) const liveErrors = plan?.outcome === 'invalid' ? plan.errors : []; @@ -263,13 +250,11 @@ function WritableBody({ (error) => error.field === binding.field, )?.message; const applied = settingsApplied(state); - const revision = settingsRevisionId(state.authority, scope); const proposed = draftValue(state.draft, binding); const edited = fieldEdited(state.draft, state.authority, binding); const requirement = applyRequirement(binding); - const staged = scopeReview !== null; - const applying = state.status === 'submitting'; - const otherReview = review !== null && scopeReview === null ? review.scope : null; + const savePending = state.status !== 'editing'; + const otherSubmission = submission !== null && scopeSubmission === null ? submission.scope : null; return ( <> @@ -278,13 +263,13 @@ function WritableBody({ authoritative - + editor.edit(binding, value)} /> @@ -292,16 +277,6 @@ function WritableBody({ - - {revision} - - {scopeReview - ? scopeReview.expectedRevisionId === revision - ? 'held by this review' - : `review held ${scopeReview.expectedRevisionId}` - : 'checked again immediately before apply'} - - {scopeNoun(scope)} @@ -313,10 +288,10 @@ function WritableBody({ - {otherReview ? ( -

- A {scopeNoun(otherReview)} review is {reviewStatusWord(state.status)}. Editing this - value withdraws it. + {otherSubmission ? ( +

+ A {scopeNoun(otherSubmission)} save is {saveStatusWord(state.status)}. This draft is + unchanged.

) : null} @@ -347,31 +322,25 @@ function WritableBody({

) : null} - {scopeReview ? ( - + {scopeSubmission ? ( + ) : (
)} @@ -379,82 +348,41 @@ function WritableBody({ ); } -/** - * Once the change is frozen against a revision the panel shows exactly what - * will be sent, asks for confirmation against that revision, and then renders - * whatever verdict came back, each as its own state, none as "the save failed". - */ -function FrozenReview({ +/** Keep the submitted intent visible until it has a truthful outcome. */ +function SubmissionStatus({ state, - patch, - expectedRevisionId, scope, editor, }: { state: SettingsEditorState; - patch: unknown; - expectedRevisionId: string; scope: SettingsScope; editor: SettingsEditorHandle; }) { - const confirmed = settingsConfirmationHeld(state); - const applying = state.status === 'submitting'; - const resolvable = state.status === 'conflicted' - || state.status === 'review_superseded' - || (state.status === 'submit_failed' - && (state.failure.kind === 'protocol_error' || state.failure.kind === 'unsettled')); - const retryable = state.status === 'authority_withdrawn' || state.status === 'submit_failed'; + const requiresReload = + state.status === 'conflicted' || + (state.status === 'submit_failed' && + (state.failure.kind === 'protocol_error' || state.failure.kind === 'unsettled')); + const retryable = + state.status === 'authority_withdrawn' || + (state.status === 'submit_failed' && + (state.failure.kind === 'offline' || state.failure.kind === 'error')); return ( -
-
- validated patch - 
-          {JSON.stringify(patch, null, 2)}
-
- - only the validated changed fields above are sent · expected revision {expectedRevisionId} - -
- +
- - {resolvable ? ( + {requiresReload ? ( - ) : ( + ) : retryable ? ( - )} + ) : null}
); @@ -471,17 +399,8 @@ function Verdict({ state }: { state: SettingsEditorState }) { case 'conflicted': return ( - Another writer saved {state.review.scope} settings after this form loaded. Your draft - was based on {state.conflict.expectedRevisionId}; the current authority is{' '} - {state.conflict.actualRevisionId ?? 'unknown'}. Nothing was applied. - - ); - case 'review_superseded': - return ( - - The {state.review.scope} settings authority moved from {state.review.expectedRevisionId}{' '} - to {state.currentRevisionId} while this change was under review, so this change no - longer describes it. Nothing was sent. + Settings changed since you loaded them. Your save was not applied. Load the current + values, review your changes, and save again. ); case 'authority_withdrawn': @@ -489,15 +408,15 @@ function Verdict({ state }: { state: SettingsEditorState }) { case 'submit_failed': return ( - {state.failure.detail} + {state.failure.kind === 'unsettled' + ? `${state.failure.detail} Load current values before trying again.` + : state.failure.detail} ); case 'submitting': - return Applying against the held revision…; + return Saving settings…; case 'editor_unavailable': case 'editing': - case 'reviewing': - case 'confirmed': return null; default: { const exhaustive: never = state; diff --git a/dashboard/src/workspaces/settings/SettingsInspector.tsx b/dashboard/src/workspaces/settings/SettingsInspector.tsx index 31c9f90fab..4b6b1c2199 100644 --- a/dashboard/src/workspaces/settings/SettingsInspector.tsx +++ b/dashboard/src/workspaces/settings/SettingsInspector.tsx @@ -21,7 +21,7 @@ import { formatMicrosUtc } from '../../ui/format.ts'; import { StateChip } from '../../ui/StateChip.tsx'; import { MultiRootPanel } from './MultiRootPanel.tsx'; import { RemoteBrainPanel } from './RemoteBrainPanel.tsx'; -import { settingsReviewOf, type SettingsEditorState } from './settingsEditorMachine.ts'; +import { settingsSubmissionOf, type SettingsEditorState } from './settingsEditorMachine.ts'; import type { WritableScopes } from './settingsGates.ts'; import { settingsRevisionId, type SettingsModel } from './settingsModel.ts'; import { @@ -39,7 +39,7 @@ import { ValueCell, WriteCell, provenanceSentence, - reviewStatusWord, + saveStatusWord, type RowProvenance, } from './SettingsValues.tsx'; @@ -64,13 +64,13 @@ export function SettingsInspector({ }) { return (
-
+
{row ? 'inspecting' : 'configuration snapshot'} -

Provenance / review

+

Configuration details
@@ -185,7 +185,7 @@ function writeSentence( case 'locked': return capability.reason; case 'no_write_path': - return 'No settings PATCH route addresses this key; it is reported, not edited, here.'; + return 'This value is read-only here. Change it in the configuration source that owns it.'; default: { const exhaustive: never = capability; return exhaustive; @@ -257,7 +257,7 @@ function SnapshotFacts({ - + {model.overrides.length > 0 ? ( @@ -267,9 +267,8 @@ function SnapshotFacts({ ) : null} - A proposal is validated against the revision it was planned from and checked again - immediately before apply. If the revision changes first, the patch is rejected as a - conflict and nothing is written. + If settings change while you edit, Save stops and asks you to load current values. + Your changes are never silently written over newer settings. @@ -278,20 +277,16 @@ function SnapshotFacts({ } /** - * The one review the editor can hold, stated here so a verdict, a conflict, - * a withdrawn authority, a failed write, stays visible after its row's panel - * is closed, rather than surviving only as a word in the register. + * A save's outcome stays visible after its row's editor is closed. */ -function HeldReviewFact({ state }: { state: SettingsEditorState }) { - const review = settingsReviewOf(state); - if (review === null) return null; +function PendingSaveFact({ state }: { state: SettingsEditorState }) { + const submission = settingsSubmissionOf(state); + if (submission === null) return null; return ( - - - {reviewStatusWord(state.status)} ·{' '} - {scopeNoun(review.scope)} change against revision{' '} - {review.expectedRevisionId}. Select a{' '} - {scopeNoun(review.scope)} key to resolve it. + + + {saveStatusWord(state.status)} ·{' '} + {scopeNoun(submission.scope)} settings. Select a matching setting to see the next action. ); diff --git a/dashboard/src/workspaces/settings/SettingsPage.dom.test.tsx b/dashboard/src/workspaces/settings/SettingsPage.dom.test.tsx index 14fd52604d..1935e8eb13 100644 --- a/dashboard/src/workspaces/settings/SettingsPage.dom.test.tsx +++ b/dashboard/src/workspaces/settings/SettingsPage.dom.test.tsx @@ -105,7 +105,7 @@ function projectPatchResponse(current: unknown) { /** * These suites assert the exact request sequence the settings write protocol - * performs: read, re-read for the confirmation, patch, refresh. The page also + * performs: initial read, preflight read, patch, and refresh. The page also * reads `/api/capabilities` and `/api/remote/status` for the inspector, which * are neither part of that protocol nor able to affect it, so the recorders * below keep only settings traffic. @@ -126,20 +126,20 @@ async function findRow(key: string): Promise { return waitFor(() => row(key)); } -/** Select a row and wait for its review to open under it. */ -async function openReview(user: ReturnType, key: string) { +/** Select a row and wait for its editor to open under it. */ +async function openEditor(user: ReturnType, key: string) { await user.click(await findRow(key)); return waitFor(() => { - const panel = document.querySelector(`[data-settings-review="${key}"]`); - if (!panel) throw new Error(`no review open for ${key}`); + const panel = document.querySelector(`[data-settings-editor="${key}"]`); + if (!panel) throw new Error(`no editor open for ${key}`); return panel; }); } -const proposal = (key: string) => screen.getByLabelText(`Proposed value for ${key}`); -const reviewReadout = () => document.querySelector('[data-review]')?.getAttribute('data-review'); +const proposal = (key: string) => screen.getByLabelText(`Edit ${key}`); +const saveReadout = () => document.querySelector('[data-save-status]')?.getAttribute('data-save-status'); -describe('SettingsPage effective configuration review', () => { +describe('SettingsPage direct settings editing', () => { beforeEach(() => { vi.restoreAllMocks(); useScope.getState().selectAllProjects(); @@ -163,7 +163,7 @@ describe('SettingsPage effective configuration review', () => { expect(within(inspection).getByText('reported on apply')).toBeTruthy(); // Inspecting is not selecting: no review opened, the row is not pressed. expect(target.getAttribute('aria-selected')).toBe('false'); - expect(document.querySelector('[data-settings-review]')).toBeNull(); + expect(document.querySelector('[data-settings-editor]')).toBeNull(); // Focus is the keyboard's hover. act(() => row(POLL_SECS).focus()); @@ -208,7 +208,7 @@ describe('SettingsPage effective configuration review', () => { expect(within(row('storage.store_root')).getByText('origin not served')).toBeTruthy(); }); - it('reviews and applies a project patch with the held revision', async () => { + it('saves a project edit directly with its current revision and reports the result', async () => { const calls: Array<{ url: string; method: string; body: unknown }> = []; let applied = false; vi.stubGlobal( @@ -233,9 +233,8 @@ describe('SettingsPage effective configuration review', () => { const user = userEvent.setup(); renderSettings(); - const panel = await openReview(user, MAX_FILE_SIZE); - expect(reviewReadout()).toBe('none'); - expect(within(panel).getByText('rev-42')).toBeTruthy(); + const panel = await openEditor(user, MAX_FILE_SIZE); + expect(saveReadout()).toBe('none'); expect(within(panel).getByText('applies to the active project')).toBeTruthy(); const input = proposal(MAX_FILE_SIZE); @@ -246,28 +245,22 @@ describe('SettingsPage effective configuration review', () => { expect(within(row(MAX_FILE_SIZE)).getByText('1,048,576')).toBeTruthy(); expect(within(row(MAX_FILE_SIZE)).getByText('2,097,152')).toBeTruthy(); expect(document.querySelector('[data-settings-validation]')?.getAttribute('data-settings-validation')).toBe('ready'); - expect(reviewReadout()).toBe('proposal'); - - await user.click(within(panel).getByRole('button', { name: 'Review project change' })); - expect(reviewReadout()).toBe('pending'); - expect(within(panel).getByText(/"max_file_size": 2097152/)).toBeTruthy(); - expect(within(panel).getByRole('button', { name: 'Apply project settings' }).hasAttribute('disabled')).toBe(true); - await user.click( - within(panel).getByRole('checkbox', { - name: /I confirm this change against configuration revision rev-42/, - }), - ); - await user.click(within(panel).getByRole('button', { name: 'Apply project settings' })); + expect(saveReadout()).toBe('unsaved'); + expect(within(panel).queryByRole('checkbox')).toBeNull(); + expect(within(panel).queryByRole('button', { name: /Review/ })).toBeNull(); + const save = within(panel).getByRole('button', { name: 'Save' }); + save.focus(); + await user.keyboard('{Enter}'); expect(await screen.findByText('Project settings saved')).toBeTruthy(); expect(screen.getByText('Receipt reconciliation pending')).toBeTruthy(); expect(screen.getByText('Resync recommended')).toBeTruthy(); - expect(reviewReadout()).toBe('applied'); + expect(saveReadout()).toBe('applied'); // The effective value is the read-back, and the proposal is gone. await waitFor(() => expect(row(MAX_FILE_SIZE).dataset['provenance']).toBe('unserved')); expect(within(row(MAX_FILE_SIZE)).getByText('2,097,152')).toBeTruthy(); expect(within(panel).getByText('revision now rev-43')).toBeTruthy(); - // Focus returned to the edited row once the review resolved. + // Focus returns to the edited row after the save resolves. expect(document.activeElement).toBe(row(MAX_FILE_SIZE)); expect(calls.map(({ method, url }) => `${method} ${url}`)).toEqual([ 'GET /api/settings', @@ -282,7 +275,7 @@ describe('SettingsPage effective configuration review', () => { }); }); - it('round trips an exact code-index worker selection through review, PATCH, and refresh', async () => { + it('saves a worker selection through its profile revision and refreshes it', async () => { useScope.setState({ scope: { kind: 'project', @@ -321,7 +314,7 @@ describe('SettingsPage effective configuration review', () => { expect(within(row('user.code_index_worker_status.limiting_reason')).getByText('automatic_all_cores')).toBeTruthy(); expect(within(row(WORKERS)).getByText('automatic')).toBeTruthy(); - const panel = await openReview(user, WORKERS); + const panel = await openEditor(user, WORKERS); expect(within(panel).getByText('daemon restart')).toBeTruthy(); expect(within(panel).getByText('applies to your TraceDecay profile')).toBeTruthy(); await user.click(screen.getByLabelText('Exact number of cores')); @@ -329,14 +322,7 @@ describe('SettingsPage effective configuration review', () => { await user.clear(workers); await user.type(workers, '4'); expect(within(row(WORKERS)).getByText('exact · 4 workers')).toBeTruthy(); - await user.click(screen.getByRole('button', { name: 'Review code-index worker change' })); - expect(within(panel).getByText(/"mode": "exact"/)).toBeTruthy(); - await user.click( - screen.getByRole('checkbox', { - name: /I confirm this change against configuration revision profile-worker-rev-7/, - }), - ); - await user.click(screen.getByRole('button', { name: 'Apply code-index worker selection' })); + await user.click(screen.getByRole('button', { name: 'Save' })); expect(await screen.findByText('Code-index worker selection saved')).toBeTruthy(); expect(screen.getByText('Restart recommended')).toBeTruthy(); @@ -372,7 +358,7 @@ describe('SettingsPage effective configuration review', () => { renderSettings(); expect(within(await findRow('user.code_index_worker_status.environment_override_workers')).getByText('7')).toBeTruthy(); - await openReview(events, WORKERS); + await openEditor(events, WORKERS); expect( screen.getByText( 'TRACEDECAY_INDEX_WORKERS=7 overrides the persisted worker selection for this running daemon.', @@ -388,7 +374,7 @@ describe('SettingsPage effective configuration review', () => { const events = userEvent.setup(); renderSettings(); - await openReview(events, WORKERS); + await openEditor(events, WORKERS); expect( screen.getByText( 'Current CPU and memory admission limits are unavailable; an exact count is judged when the daemon restarts.', @@ -410,12 +396,10 @@ describe('SettingsPage effective configuration review', () => { vi.stubGlobal('fetch', fetch); const user = userEvent.setup(); renderSettings(); - await openReview(user, MAX_FILE_SIZE); + await openEditor(user, MAX_FILE_SIZE); await user.clear(proposal(MAX_FILE_SIZE)); await user.type(proposal(MAX_FILE_SIZE), '2097152'); - await user.click(screen.getByRole('button', { name: 'Review project change' })); - await user.click(screen.getByRole('checkbox', { name: /I confirm this change against configuration revision rev-42/ })); - await user.click(screen.getByRole('button', { name: 'Apply project settings' })); + await user.click(screen.getByRole('button', { name: 'Save' })); expect(await screen.findByText(/Refresh current values before retrying/)).toBeTruthy(); expect(screen.queryByText('Project settings saved')).toBeNull(); expect(screen.queryByRole('button', { name: 'Retry project settings' })).toBeNull(); @@ -449,24 +433,18 @@ describe('SettingsPage effective configuration review', () => { const user = userEvent.setup(); renderSettings(); - await openReview(user, MAX_FILE_SIZE); + await openEditor(user, MAX_FILE_SIZE); const input = proposal(MAX_FILE_SIZE); await user.clear(input); await user.type(input, '2097152'); - await user.click(screen.getByRole('button', { name: 'Review project change' })); - await user.click( - screen.getByRole('checkbox', { - name: /I confirm this change against configuration revision rev-42/, - }), - ); - await user.click(screen.getByRole('button', { name: 'Apply project settings' })); + await user.click(screen.getByRole('button', { name: 'Save' })); expect( await screen.findByText( - 'Another writer saved project settings after this form loaded. Your draft was based on rev-42; the current authority is rev-43. Nothing was applied.', + 'Settings changed since you loaded them. Your save was not applied. Load the current values, review your changes, and save again.', ), ).toBeTruthy(); - expect(reviewReadout()).toBe('conflict'); + expect(saveReadout()).toBe('conflict'); await user.click(screen.getByRole('button', { name: 'Load current values' })); expect(await screen.findByDisplayValue('4096')).toBeTruthy(); expect(within(row(MAX_FILE_SIZE)).getByText('4,096')).toBeTruthy(); @@ -491,15 +469,16 @@ describe('SettingsPage effective configuration review', () => { // The worker resource keeps its distinct ProfileSessions authority. expect(row(WORKERS).dataset['write']).toBe('writable'); - const panel = await openReview(user, MAX_FILE_SIZE); - expect(within(panel).getByText(/this dashboard is not authorized to apply project settings/)).toBeTruthy(); + const panel = await openEditor(user, MAX_FILE_SIZE); + expect(within(panel).getByText(/cannot change project settings\. Ask an administrator/)).toBeTruthy(); expect(panel.querySelector('[data-settings-gate="unauthorized"]')).toBeTruthy(); - expect(screen.queryByLabelText(`Proposed value for ${MAX_FILE_SIZE}`)).toBeNull(); - expect(screen.queryByRole('button', { name: 'Review project change' })).toBeNull(); + expect(screen.queryByLabelText(`Edit ${MAX_FILE_SIZE}`)).toBeNull(); + expect(screen.queryByRole('button', { name: 'Save' })).toBeNull(); expect(calls).toEqual(['GET /api/settings']); }); it('reports a withdrawn authority as unavailable rather than a failed write', async () => { + const patchKeys: string[] = []; vi.stubGlobal( 'fetch', vi.fn(async (input: RequestInfo | URL, init?: RequestInit) => { @@ -507,6 +486,8 @@ describe('SettingsPage effective configuration review', () => { const method = init?.method ?? 'GET'; if (url === '/api/settings' && method === 'GET') return jsonResponse(settings()); if (url === '/api/settings/project' && method === 'PATCH') { + const body = JSON.parse(String(init?.body)) as { idempotency_key: string }; + patchKeys.push(body.idempotency_key); return jsonResponse( { code: 'configuration_authority_unavailable', @@ -521,24 +502,22 @@ describe('SettingsPage effective configuration review', () => { const user = userEvent.setup(); renderSettings(); - await openReview(user, MAX_FILE_SIZE); + await openEditor(user, MAX_FILE_SIZE); const input = proposal(MAX_FILE_SIZE); await user.clear(input); await user.type(input, '2097152'); - await user.click(screen.getByRole('button', { name: 'Review project change' })); - await user.click( - screen.getByRole('checkbox', { - name: /I confirm this change against configuration revision rev-42/, - }), - ); - await user.click(screen.getByRole('button', { name: 'Apply project settings' })); + await user.click(screen.getByRole('button', { name: 'Save' })); expect( await screen.findByText('Nothing was applied: configuration authority is unavailable.'), ).toBeTruthy(); - expect(reviewReadout()).toBe('withdrawn'); + expect(saveReadout()).toBe('unavailable'); expect(screen.getByRole('button', { name: 'Retry project settings' })).toBeTruthy(); expect(screen.queryByText('Project settings saved')).toBeNull(); + + await user.click(screen.getByRole('button', { name: 'Retry project settings' })); + await waitFor(() => expect(patchKeys).toHaveLength(2)); + expect(patchKeys[1]).toBe(patchKeys[0]); }); it('shows client validation live and never sends an invalid patch', async () => { @@ -554,7 +533,7 @@ describe('SettingsPage effective configuration review', () => { const user = userEvent.setup(); renderSettings(); - await openReview(user, POLL_SECS); + await openEditor(user, POLL_SECS); const poll = proposal(POLL_SECS); await user.clear(poll); await user.type(poll, '59'); @@ -567,7 +546,7 @@ describe('SettingsPage effective configuration review', () => { expect(document.getElementById(pollError ?? '')?.textContent).toBe( 'auto_track_pr_poll_secs must be at least 60 seconds', ); - expect(screen.getByRole('button', { name: 'Review project change' }).hasAttribute('disabled')).toBe(true); + expect(screen.getByRole('button', { name: 'Save' }).hasAttribute('disabled')).toBe(true); expect(calls).toEqual(['GET /api/settings']); }); @@ -603,31 +582,25 @@ describe('SettingsPage effective configuration review', () => { const user = userEvent.setup(); renderSettings(); - await openReview(user, WATCHER_DEBOUNCE); + await openEditor(user, WATCHER_DEBOUNCE); const debounce = proposal(WATCHER_DEBOUNCE); await user.clear(debounce); await user.type(debounce, '15s'); - await user.click(screen.getByRole('button', { name: 'Review user change' })); - await user.click( - screen.getByRole('checkbox', { - name: /I confirm this change against configuration revision user-rev-7/, - }), - ); - await user.click(screen.getByRole('button', { name: 'Apply user settings' })); + await user.click(screen.getByRole('button', { name: 'Save' })); expect( (await screen.findAllByText('watcher debounce is denied by the active profile policy')).length, ).toBeGreaterThan(0); expect(screen.getByText(/The daemon rejected this user settings change/)).toBeTruthy(); - expect(reviewReadout()).toBe('rejected'); + expect(saveReadout()).toBe('rejected'); expect(debounce.getAttribute('aria-invalid')).toBe('true'); const debounceError = debounce.getAttribute('aria-describedby'); expect(debounceError).not.toBeNull(); expect(document.getElementById(debounceError ?? '')?.textContent).toBe( 'watcher debounce is denied by the active profile policy', ); - // The frozen review is gone; the draft is intact for another attempt. - expect(document.querySelector('[data-settings-stage]')).toBeNull(); + // The draft remains editable after server validation. + expect(document.querySelector('[data-settings-save]')).toBeNull(); expect(calls).toEqual([ { method: 'GET', url: '/api/settings', body: null }, { method: 'GET', url: '/api/settings', body: null }, @@ -643,7 +616,7 @@ describe('SettingsPage effective configuration review', () => { ]); }); - it('keeps a verdict visible after its panel is closed, and lets a locked scope release it', async () => { + it('keeps a conflict visible after the editor closes until current values are loaded', async () => { let getCount = 0; let authorityWithdrawn = false; vi.stubGlobal( @@ -665,45 +638,29 @@ describe('SettingsPage effective configuration review', () => { const user = userEvent.setup(); const { client } = renderSettings(); - await openReview(user, MAX_FILE_SIZE); + await openEditor(user, MAX_FILE_SIZE); const input = proposal(MAX_FILE_SIZE); await user.clear(input); await user.type(input, '2097152'); - await user.click(screen.getByRole('button', { name: 'Review project change' })); - await user.click( - screen.getByRole('checkbox', { - name: /I confirm this change against configuration revision rev-42/, - }), - ); - await user.click(screen.getByRole('button', { name: 'Apply project settings' })); - await screen.findByText(/Another writer saved project settings/); + await user.click(screen.getByRole('button', { name: 'Save' })); + await screen.findByText(/Settings changed since you loaded them/); - // Closing the panel does not resolve the review; the inspector still - // states it, and another scope's row names the real status. + // Closing the editor does not hide the conflict from the inspector. await user.keyboard('{Escape}'); - expect(document.querySelector('[data-settings-review]')).toBeNull(); - const held = document.querySelector('[data-settings-held-review]'); - expect(held?.getAttribute('data-settings-held-review')).toBe('conflicted'); - expect(held?.textContent).toContain('in conflict'); - expect(held?.textContent).toContain('rev-42'); - await openReview(user, WATCHER_DEBOUNCE); - expect(document.querySelector('[data-settings-other-review]')?.textContent).toContain( - 'A project review is in conflict', + expect(document.querySelector('[data-settings-editor]')).toBeNull(); + expect(document.querySelector('[data-settings-save-status="conflicted"]')?.textContent).toContain( + 'conflict', ); - // The daemon withdraws the configuration effect while the verdict is held: - // the refetch moves the authority (the review is superseded) and locks the - // scope. The locked panel cannot apply, but it can still let the review go. + // The authority may become read-only while the conflict is held. The only + // available resolution is to load its current values. authorityWithdrawn = true; await act(() => client.invalidateQueries({ queryKey: ['settings'] })); await waitFor(() => expect(row(MAX_FILE_SIZE).dataset['write']).toBe('locked')); - await user.keyboard('{Escape}'); - const locked = await openReview(user, MAX_FILE_SIZE); + const locked = await openEditor(user, MAX_FILE_SIZE); expect(locked.querySelector('[data-settings-gate="unauthorized"]')).toBeTruthy(); - expect(locked.querySelector('[data-settings-stage="review_superseded"]')).toBeTruthy(); - await user.click(within(locked).getByRole('button', { name: 'Cancel review' })); - expect(document.querySelector('[data-settings-held-review]')).toBeNull(); - expect(document.querySelector('[data-settings-stage]')).toBeNull(); + await user.click(within(locked).getByRole('button', { name: 'Load current values' })); + expect(document.querySelector('[data-settings-save-status]')).toBeNull(); }); it('lets a fresh proposal take over from the last receipt', async () => { @@ -726,28 +683,22 @@ describe('SettingsPage effective configuration review', () => { const user = userEvent.setup(); renderSettings(); - await openReview(user, MAX_FILE_SIZE); + await openEditor(user, MAX_FILE_SIZE); const input = proposal(MAX_FILE_SIZE); await user.clear(input); await user.type(input, '2097152'); - await user.click(screen.getByRole('button', { name: 'Review project change' })); - await user.click( - screen.getByRole('checkbox', { - name: /I confirm this change against configuration revision rev-42/, - }), - ); - await user.click(screen.getByRole('button', { name: 'Apply project settings' })); + await user.click(screen.getByRole('button', { name: 'Save' })); expect(await screen.findByText('Project settings saved')).toBeTruthy(); await waitFor(() => expect(row(MAX_FILE_SIZE).dataset['provenance']).toBe('unserved')); await user.clear(proposal(MAX_FILE_SIZE)); await user.type(proposal(MAX_FILE_SIZE), '4096'); expect(screen.queryByText('Project settings saved')).toBeNull(); - expect(reviewReadout()).toBe('proposal'); + expect(saveReadout()).toBe('unsaved'); expect(row(MAX_FILE_SIZE).dataset['provenance']).toBe('edited'); }); - it('pins the review while its write is in flight so the verdict cannot be hidden', async () => { + it('keeps the pending save visible and prevents edits until it settles', async () => { let releasePatch: (() => void) | null = null; vi.stubGlobal( 'fetch', @@ -770,32 +721,26 @@ describe('SettingsPage effective configuration review', () => { const user = userEvent.setup(); renderSettings(); - await openReview(user, MAX_FILE_SIZE); + await openEditor(user, MAX_FILE_SIZE); const input = proposal(MAX_FILE_SIZE); await user.clear(input); await user.type(input, '2097152'); - await user.click(screen.getByRole('button', { name: 'Review project change' })); - await user.click( - screen.getByRole('checkbox', { - name: /I confirm this change against configuration revision rev-42/, - }), - ); - await user.click(screen.getByRole('button', { name: 'Apply project settings' })); - await waitFor(() => expect(reviewReadout()).toBe('applying')); + await user.click(screen.getByRole('button', { name: 'Save' })); + await waitFor(() => expect(saveReadout()).toBe('saving')); // Neither the close control, Escape, nor selecting another row may take // the panel away while the write is out. - expect(screen.getByRole('button', { name: 'Close review' }).hasAttribute('disabled')).toBe(true); + expect(screen.getByRole('button', { name: 'Close editor' }).hasAttribute('disabled')).toBe(true); await user.keyboard('{Escape}'); await user.click(row(POLL_SECS)); - expect(document.querySelector(`[data-settings-review="${MAX_FILE_SIZE}"]`)).toBeTruthy(); + expect(document.querySelector(`[data-settings-editor="${MAX_FILE_SIZE}"]`)).toBeTruthy(); expect(row(POLL_SECS).getAttribute('aria-selected')).toBe('false'); act(() => releasePatch?.()); expect( await screen.findByText('Nothing was applied: configuration authority is unavailable.'), ).toBeTruthy(); - expect(reviewReadout()).toBe('withdrawn'); + expect(saveReadout()).toBe('unavailable'); }); it('states a key without a write path as read-only rather than locked or denied', async () => { @@ -804,14 +749,14 @@ describe('SettingsPage effective configuration review', () => { renderSettings(); expect((await findRow('storage.store_root')).dataset['write']).toBe('no_write_path'); - const panel = await openReview(user, 'storage.store_root'); + const panel = await openEditor(user, 'storage.store_root'); expect(panel.querySelector('[data-settings-gate="no_write_path"]')).toBeTruthy(); - expect(within(panel).getByText(/no settings PATCH route addresses it/)).toBeTruthy(); - expect(within(panel).queryByRole('button', { name: /Review/ })).toBeNull(); + expect(within(panel).getByText(/not editable here/i)).toBeTruthy(); + expect(within(panel).queryByRole('button', { name: 'Save' })).toBeNull(); expect(within(panel).queryByRole('textbox')).toBeNull(); }); - it('opens a review from the keyboard, closes it with Escape, and returns focus to the row', async () => { + it('opens the editor from the keyboard and closes it with Escape', async () => { vi.stubGlobal('fetch', vi.fn(async () => jsonResponse(settings()))); const user = userEvent.setup(); renderSettings(); @@ -824,11 +769,11 @@ describe('SettingsPage effective configuration review', () => { expect(second.dataset['key']).not.toBe('project.config.context_scout'); await user.keyboard('{Enter}'); const key = second.dataset['key'] ?? ''; - expect(document.querySelector(`[data-settings-review="${key}"]`)).toBeTruthy(); + expect(document.querySelector(`[data-settings-editor="${key}"]`)).toBeTruthy(); expect(second.getAttribute('aria-selected')).toBe('true'); await user.keyboard('{Escape}'); - expect(document.querySelector('[data-settings-review]')).toBeNull(); + expect(document.querySelector('[data-settings-editor]')).toBeNull(); expect(document.activeElement).toBe(row(key)); }); @@ -899,7 +844,7 @@ describe('Settings scope authority', () => { expect(row(WORKERS).dataset['write']).toBe('writable'); expect(document.querySelector('[data-settings-scope="read_only"]')).toBeTruthy(); - const panel = await openReview(user, MAX_FILE_SIZE); + const panel = await openEditor(user, MAX_FILE_SIZE); const gate = panel.querySelector('[data-settings-gate="read_only"]'); expect(gate?.textContent).toContain('is not the active project'); expect(gate?.textContent).toContain('Switch scope to the active project'); @@ -925,9 +870,9 @@ describe('Settings scope authority', () => { expect((await findRow(WATCHER_DEBOUNCE)).dataset['write']).toBe('locked'); expect(row(WORKERS).dataset['write']).toBe('writable'); - const panel = await openReview(user, WATCHER_DEBOUNCE); + const panel = await openEditor(user, WATCHER_DEBOUNCE); expect(panel.querySelector('[data-settings-gate="unauthorized"]')?.textContent).toContain( - 'not authorized', + 'cannot change', ); }); @@ -945,7 +890,7 @@ describe('Settings scope authority', () => { renderSettings(); expect((await findRow(MAX_FILE_SIZE)).dataset['write']).toBe('locked'); - const panel = await openReview(user, MAX_FILE_SIZE); + const panel = await openEditor(user, MAX_FILE_SIZE); const gate = panel.querySelector('[data-settings-gate="unknown"]'); expect(gate?.textContent).toContain('not known yet'); expect(gate?.textContent).not.toMatch(/not authorized|read-only project/i); @@ -965,9 +910,9 @@ describe('Settings scope authority', () => { const user = userEvent.setup(); renderSettings(); - let panel = await openReview(user, MAX_FILE_SIZE); + let panel = await openEditor(user, MAX_FILE_SIZE); expect(within(panel).getByText('applies to Active project')).toBeTruthy(); - panel = await openReview(user, WORKERS); + panel = await openEditor(user, WORKERS); expect(within(panel).getByText('applies to your TraceDecay profile')).toBeTruthy(); }); @@ -992,7 +937,7 @@ describe('Settings scope authority', () => { ); await waitFor(() => expect(row(MAX_FILE_SIZE).dataset['write']).toBe('writable')); - const panel = await openReview(user, MAX_FILE_SIZE); + const panel = await openEditor(user, MAX_FILE_SIZE); expect(within(panel).getByText('applies to Production')).toBeTruthy(); expect(document.body.textContent).not.toContain('Scratch sandbox'); }); @@ -1008,9 +953,9 @@ describe('Settings scope authority', () => { expect((await findRow(WORKERS)).dataset['write']).toBe('locked'); expect(row(MAX_FILE_SIZE).dataset['write']).toBe('writable'); expect(row(WATCHER_DEBOUNCE).dataset['write']).toBe('writable'); - const panel = await openReview(user, WORKERS); + const panel = await openEditor(user, WORKERS); expect( - within(panel).getByText(/this dashboard is not authorized to apply code-index worker settings/), + within(panel).getByText(/cannot change code-index worker settings\. Ask an administrator/), ).toBeTruthy(); }); }); @@ -1147,19 +1092,19 @@ describe('Settings response authority', () => { const user = userEvent.setup(); renderSettings(); - expect(reviewReadout()).toBeUndefined(); + expect(saveReadout()).toBeUndefined(); await findRow(MAX_FILE_SIZE); - expect(reviewReadout()).toBe('unavailable'); + expect(saveReadout()).toBe('unavailable'); // Every bound key is locked, by the read rather than by a scope gate; an // unbound key still has no write path. expect(row(MAX_FILE_SIZE).dataset['write']).toBe('locked'); expect(row(WORKERS).dataset['write']).toBe('locked'); expect(row('storage.store_root').dataset['write']).toBe('no_write_path'); - const panel = await openReview(user, MAX_FILE_SIZE); + const panel = await openEditor(user, MAX_FILE_SIZE); expect(panel.querySelector('[data-settings-gate="editor_unavailable"]')?.textContent).toContain( 'GET /api/settings named no configuration revision to hold a write against', ); - expect(screen.queryByLabelText(`Proposed value for ${MAX_FILE_SIZE}`)).toBeNull(); + expect(screen.queryByLabelText(`Edit ${MAX_FILE_SIZE}`)).toBeNull(); // The inspector prints the empty revision as a stated absence. expect(screen.getAllByText('not stated').length).toBeGreaterThan(0); }); diff --git a/dashboard/src/workspaces/settings/SettingsPage.tsx b/dashboard/src/workspaces/settings/SettingsPage.tsx index 2cb2f6e212..770ac3c9a6 100644 --- a/dashboard/src/workspaces/settings/SettingsPage.tsx +++ b/dashboard/src/workspaces/settings/SettingsPage.tsx @@ -32,7 +32,7 @@ import { effectiveRows, filterEffectiveRows, type EffectiveRow } from './setting import { SectionsRail } from './SettingsSections.tsx'; /** - * Settings: the effective-configuration review. + * Settings: effective configuration with direct, revision-checked editing. * * `/api/settings` reports effective values. It does not attribute individual * keys to the layer that set them, and its groups do not address a shared key @@ -44,10 +44,10 @@ import { SectionsRail } from './SettingsSections.tsx'; * `edited` for this reader's own unapplied proposal; * - ORIGIN per group: the file path or endpoint the payload names, or a * stated absence; - * - WRITE per key: editable through the compare-and-swap PATCH path, locked + * - WRITE per key: editable when authorized, locked * by a named gate, or without any write path at all. * - * Hover and focus inspect; click selects and opens the review under the row. + * Hover and focus inspect; click selects and opens the editor under the row. * The effective value stays authoritative until the write authority validates, * persists against the held revision, reports its apply requirement, and the * read comes back. @@ -173,7 +173,7 @@ function SettingsSurface({ // leading slash here silently falls through to the `--` fallback. path="settings" title="Settings" - note="effective configuration review" + note="effective configuration" /> Showing effective configuration only. Origins are shown only when the server names them; otherwise a key's provenance is unserved. Hover - or focus a row to inspect it; Enter or click opens its review; Escape closes. + or focus a row to inspect it; Enter or click opens its editor; Escape closes.

@@ -275,7 +275,7 @@ function groupBySection(model: SettingsModel, rows: readonly EffectiveRow[]): Se * The register under the header: the search over key, value, and served * description, and the three readings a reader needs before touching a row , * what the scope permits, that this is the effective-only mode, and where the - * one review the editor can hold currently stands. + * the current save status. */ function SettingsRegister({ query, @@ -294,7 +294,7 @@ function SettingsRegister({ writability: ScopeWritability; state: SettingsEditorState; }) { - const review = reviewReading(state); + const save = saveReading(state); return (
@@ -339,8 +339,8 @@ function SettingsRegister({ {writability.state === 'writable' ? 'writable' : writability.state.replace('_', '-')} effective-only - - {review.value} + + {save.value}
@@ -356,7 +356,7 @@ function Register({ label: string; tone?: string; children: ReactNode; - 'data-review'?: string; + 'data-save-status'?: string; }) { return (
@@ -366,38 +366,30 @@ function Register({ ); } -/** Where the editor's one review stands, as one word the register can carry. */ -function reviewReading(state: SettingsEditorState): { value: string; tone: string } { +/** The current save outcome, as one word the register can carry. */ +function saveReading(state: SettingsEditorState): { value: string; tone: string } { switch (state.status) { case 'editor_unavailable': return { value: 'unavailable', tone: 'text-state-error' }; case 'editing': { - // A refusal always describes the current draft (an edit clears it), so - // it outranks the bare fact of a proposal; a fresh proposal outranks the - // receipt of the last write, which survives a redraft only as the - // panel's receipt. The register answers "what is pending now". + // A refusal describes the current draft; a fresh change outranks the + // receipt of the last write, which remains visible in the editor. if (settingsRejection(state)) return { value: 'rejected', tone: 'text-state-error' }; const dirty = (['project', 'user', 'code_index_workers'] as const).some((scope) => settingsScopeDirty(state, scope), ); - if (dirty) return { value: 'proposal', tone: 'text-accent' }; + if (dirty) return { value: 'unsaved', tone: 'text-accent' }; if (settingsApplied(state)) return { value: 'applied', tone: 'text-state-ready' }; return { value: 'none', tone: 'text-text-muted' }; } - case 'reviewing': - return { value: 'pending', tone: 'text-accent' }; - case 'confirmed': - return { value: 'confirmed', tone: 'text-accent' }; case 'submitting': - return { value: 'applying', tone: 'text-state-loading' }; + return { value: 'saving', tone: 'text-state-loading' }; case 'conflicted': return { value: 'conflict', tone: 'text-state-conflicting' }; - case 'review_superseded': - return { value: 'superseded', tone: 'text-state-conflicting' }; case 'authority_withdrawn': - return { value: 'withdrawn', tone: 'text-state-offline' }; + return { value: 'unavailable', tone: 'text-state-offline' }; case 'submit_failed': - return { value: 'failed', tone: 'text-state-error' }; + return { value: 'uncertain', tone: 'text-state-error' }; default: { const exhaustive: never = state; return exhaustive; diff --git a/dashboard/src/workspaces/settings/SettingsValues.tsx b/dashboard/src/workspaces/settings/SettingsValues.tsx index b574403c1a..f66be758d1 100644 --- a/dashboard/src/workspaces/settings/SettingsValues.tsx +++ b/dashboard/src/workspaces/settings/SettingsValues.tsx @@ -251,7 +251,7 @@ export function ProvenanceChip({ kind }: { kind: RowProvenance }) { /** * When a persisted change takes effect, as the product states it: the one * documented requirement in its own register, or the statement that the write - * authority reports it on apply. Shared by the review panel and the inspector. + * authority reports it on Save. Shared by the editor and the inspector. */ export function ApplyRequirementText({ requirement }: { requirement: ApplyRequirement }) { switch (requirement.kind) { @@ -269,7 +269,7 @@ export function ApplyRequirementText({ requirement }: { requirement: ApplyRequir <> reported on apply - the write authority states resync or restart requirements in its response + this page will show if a restart or resync is needed after saving ); @@ -280,25 +280,20 @@ export function ApplyRequirementText({ requirement }: { requirement: ApplyRequir } } -/** The held review's state as one word a sentence can carry. */ -export function reviewStatusWord(status: SettingsEditorState['status']): string { +/** The last save's state as one word a sentence can carry. */ +export function saveStatusWord(status: SettingsEditorState['status']): string { switch (status) { - case 'reviewing': - case 'confirmed': - return 'pending'; case 'submitting': - return 'applying'; + return 'saving'; case 'conflicted': - return 'in conflict'; - case 'review_superseded': - return 'superseded'; + return 'conflict'; case 'authority_withdrawn': - return 'withdrawn'; + return 'unavailable'; case 'submit_failed': - return 'failed'; + return 'uncertain'; case 'editor_unavailable': case 'editing': - return 'not held'; + return 'none'; default: { const exhaustive: never = status; return exhaustive; diff --git a/dashboard/src/workspaces/settings/settingsChrome.ts b/dashboard/src/workspaces/settings/settingsChrome.ts index 73e1932abc..cf0fb8f1c1 100644 --- a/dashboard/src/workspaces/settings/settingsChrome.ts +++ b/dashboard/src/workspaces/settings/settingsChrome.ts @@ -3,8 +3,7 @@ * bezel inside a larger hit area the way the panel-header controls do. `+2px` * on the input is its two hairlines, so the content box lands on 44. * - * Shared by the editor fields and the review dialog, which are one control - * surface split across two modules. */ + * Shared by the editor fields and the surrounding settings surface. */ export const settingsInputClass = 'h-[calc(var(--touch-target-min)+2px)] w-full rounded-[var(--radius-chip)] border border-edge-subtle bg-surface-0 px-2 text-xs text-text-primary outline-none focus-visible:border-accent'; diff --git a/dashboard/src/workspaces/settings/settingsEditorMachine.test.ts b/dashboard/src/workspaces/settings/settingsEditorMachine.test.ts index 351b3f0f5d..199636d3f8 100644 --- a/dashboard/src/workspaces/settings/settingsEditorMachine.test.ts +++ b/dashboard/src/workspaces/settings/settingsEditorMachine.test.ts @@ -1,27 +1,14 @@ -/** - * What the editor machine forbids. - * - * Each case below names a combination the six mirrored `useState` slices could - * hold at once, a confirmation beside a review it was not given for, a saved - * notice above a change still under review, a submit built from a revision the - * resource had already moved past, and shows either that the state cannot be - * built or that the machine refuses it and says why. - * - * No DOM, no transport: the machine is a pure function and is exercised as one. - */ - import { describe, expect, it } from 'vitest'; import { FIXTURES } from '../../../stories/fixtures/data.ts'; import { initialSettingsEditorState, reduceSettingsEditor, settingsApplied, - settingsConfirmationHeld, settingsRejection, - settingsReviewOf, settingsScopeDirty, settingsScopePlan, settingsSubmission, + settingsSubmissionOf, type SettingsEditorAction, type SettingsEditorState, type SettingsRoutes, @@ -46,61 +33,14 @@ const ROUTES: SettingsRoutes = { const AUTHORITY = fixtureAuthority(); const IDEMPOTENCY_KEY = 'idempotency.dashboard-settings.fixture'; -describe('settings editor: reaching a confirmed change', () => { - it('takes a validated change from editing through review to confirmation', () => { - const confirmed = run( - initialSettingsEditorState(AUTHORITY), - { type: 'project_drafted', values: draftMaxFileSize('2097152') }, - { - type: 'review_requested', - scope: 'project', - idempotencyKey: IDEMPOTENCY_KEY, - }, - { type: 'confirmation_set', confirmed: true }, - ); - - expect(confirmed.status).toBe('confirmed'); - expect(settingsConfirmationHeld(confirmed)).toBe(true); - expect(settingsReviewOf(confirmed)).toMatchObject({ - scope: 'project', - expectedRevisionId: 'rev-42', - idempotencyKey: IDEMPOTENCY_KEY, - patch: { max_file_size: 2_097_152 }, - }); - expect(settingsScopeDirty(confirmed, 'project')).toBe(true); - expect(settingsScopeDirty(confirmed, 'user')).toBe(false); - }); - - /** The inline review states validity while the value is still being typed, - * from the same plan `review_requested` would freeze, never a second one. */ - it('replans a scope live without freezing a review', () => { - const untouched = initialSettingsEditorState(AUTHORITY); - expect(settingsScopePlan(untouched, 'project')).toMatchObject({ outcome: 'unchanged' }); - expect(settingsScopePlan(initialSettingsEditorState(null), 'project')).toBeNull(); - - const edited = run(untouched, { - type: 'project_drafted', - values: draftMaxFileSize('2097152'), - }); - expect(edited.status).toBe('editing'); - expect(settingsScopePlan(edited, 'project')).toMatchObject({ - outcome: 'ready', - expectedRevisionId: 'rev-42', - patch: { max_file_size: 2_097_152 }, - }); - expect(settingsReviewOf(edited)).toBeNull(); - - const invalid = run(untouched, { type: 'project_drafted', values: draftMaxFileSize('0') }); - expect(settingsScopePlan(invalid, 'project')).toMatchObject({ - outcome: 'invalid', - errors: [{ field: 'max_file_size', message: 'max_file_size must be at least 1 byte' }], - }); - expect(settingsRejection(invalid)).toBeNull(); - }); - - it('issues a submit that names the held revision and the scope authority', () => { +describe('settings editor direct Save', () => { + it('captures the valid patch, revision, and request identity in one Save', () => { const submitting = expectSubmitting( - run(confirmedProjectChange(), { type: 'submit_started' }), + run( + initialSettingsEditorState(AUTHORITY), + { type: 'project_drafted', values: draftMaxFileSize('2097152') }, + { type: 'save_requested', scope: 'project', idempotencyKey: IDEMPOTENCY_KEY }, + ), ); expect(settingsSubmission(submitting, ROUTES)).toEqual({ @@ -111,9 +51,15 @@ describe('settings editor: reaching a confirmed change', () => { patchUrl: '/api/settings/project', patch: { max_file_size: 2_097_152 }, }); + expect(settingsSubmissionOf(submitting)).toMatchObject({ + scope: 'project', + expectedRevisionId: 'rev-42', + patch: { max_file_size: 2_097_152 }, + idempotencyKey: IDEMPOTENCY_KEY, + }); }); - it('holds a worker-only change against the independent ProfileSessions revision', () => { + it('uses the profile revision and route for worker settings', () => { const submitting = expectSubmitting( run( initialSettingsEditorState(AUTHORITY), @@ -125,12 +71,10 @@ describe('settings editor: reaching a confirmed change', () => { }, }, { - type: 'review_requested', + type: 'save_requested', scope: 'code_index_workers', idempotencyKey: IDEMPOTENCY_KEY, }, - { type: 'confirmation_set', confirmed: true }, - { type: 'submit_started' }, ), ); @@ -143,203 +87,120 @@ describe('settings editor: reaching a confirmed change', () => { patch: { code_index_workers: { mode: 'exact', workers: 4 } }, }); }); -}); -describe('settings editor: a confirmation cannot outlive its review', () => { - // Previously: `confirmed` was a boolean beside `review`. A refetch replaced - // the draft through an effect and left both untouched, so the next apply - // sent a patch the user had confirmed against a revision that was gone. - it('supersedes a confirmation when the resource moves under it', () => { - const moved = run(confirmedProjectChange(), { - type: 'authority_observed', - authority: withProjectRevision(AUTHORITY, 'rev-43'), - }); - - expect(moved.status).toBe('review_superseded'); - expect(moved).toMatchObject({ currentRevisionId: 'rev-43' }); - expect(settingsConfirmationHeld(moved)).toBe(false); - // And it stays superseded: there is no path from here to a write. - expect(run(moved, { type: 'submit_started' }).status).toBe('review_superseded'); - }); - - // Previously: the review dialog held a patch computed from a draft the - // editor had already replaced. Editing behind an open review left the two - // describing different changes. - it('drops the review entirely when the draft it described is edited', () => { - const edited = run(confirmedProjectChange(), { + it('keeps an edited draft on its original revision when a newer snapshot arrives', () => { + const drafted = run(initialSettingsEditorState(AUTHORITY), { type: 'project_drafted', - values: draftMaxFileSize('4096'), + values: draftMaxFileSize('2097152'), }); - - expect(edited.status).toBe('editing'); - expect(settingsReviewOf(edited)).toBeNull(); - expect(settingsConfirmationHeld(edited)).toBe(false); - }); - - // The revision alone is not the binding: the confirmation is for one exact - // change. A resource whose values moved without moving its revision must not - // inherit a confirmation given for the values it used to hold. - it('refuses a submit when replanning no longer produces the confirmed change', () => { - const restated = run(confirmedProjectChange(), { + const observed = run(drafted, { type: 'authority_observed', - // Same revision, but the authority now already holds the confirmed value. - authority: withProjectValues(AUTHORITY, { max_file_size: '2097152' }), + authority: withProjectRevision(AUTHORITY, 'rev-43'), }); - expect(restated.status).toBe('confirmed'); - - const attempted = run(restated, { type: 'submit_started' }); - - expect(attempted.status).toBe('review_superseded'); - expect(attempted).toMatchObject({ currentRevisionId: 'rev-42' }); - }); - - it('cannot hold a confirmation without a review to hold it for', () => { - const editing = initialSettingsEditorState(AUTHORITY); - const attempted = run(editing, { type: 'confirmation_set', confirmed: true }); - - expect(attempted).toBe(editing); - expect(settingsConfirmationHeld(attempted)).toBe(false); - expect(settingsReviewOf(attempted)).toBeNull(); - }); - - it('returns to an unconfirmed review when the confirmation is withdrawn', () => { - const withdrawn = run(confirmedProjectChange(), { - type: 'confirmation_set', - confirmed: false, + expect(observed).toBe(drafted); + expect(settingsScopePlan(observed, 'project')).toMatchObject({ + outcome: 'ready', + expectedRevisionId: 'rev-42', + patch: { max_file_size: 2_097_152 }, }); - - expect(withdrawn.status).toBe('reviewing'); - expect(settingsConfirmationHeld(withdrawn)).toBe(false); - expect(run(withdrawn, { type: 'submit_started' }).status).toBe('reviewing'); }); -}); - -describe('settings editor: a stale revision cannot reach the wire', () => { - // Even hand-built, the shape below is the one the mirrored slices produced - // routinely, a state whose review names a revision the authority has moved - // past yields no request. - it('supersedes a submit issued from a review that holds a stale revision', () => { - const stale: SettingsEditorState = { - ...confirmedProjectChange(), - authority: withProjectRevision(AUTHORITY, 'rev-43'), - }; - const attempted = run(stale, { type: 'submit_started' }); - - expect(attempted.status).not.toBe('submitting'); - expect(attempted.status).toBe('review_superseded'); - expect(attempted).toMatchObject({ - currentRevisionId: 'rev-43', - review: { expectedRevisionId: 'rev-42' }, - }); - }); + it('does not submit invalid or unchanged values', () => { + const untouched = initialSettingsEditorState(AUTHORITY); + expect( + run(untouched, { + type: 'save_requested', + scope: 'project', + idempotencyKey: IDEMPOTENCY_KEY, + }), + ).toBe(untouched); - it('offers the current values instead of a retry once a review is superseded', () => { - const superseded = run( - { ...confirmedProjectChange(), authority: withProjectRevision(AUTHORITY, 'rev-43') }, - { type: 'submit_started' }, - { type: 'reloaded_from_authority' }, + const invalid = run( + untouched, + { type: 'project_drafted', values: draftMaxFileSize('0') }, + { type: 'save_requested', scope: 'project', idempotencyKey: IDEMPOTENCY_KEY }, ); - - expect(superseded.status).toBe('editing'); - expect(settingsReviewOf(superseded)).toBeNull(); - expect(settingsScopeDirty(superseded, 'project')).toBe(false); + expect(invalid.status).toBe('editing'); + expect(settingsRejection(invalid)?.errors).toEqual([ + { field: 'max_file_size', message: 'max_file_size must be at least 1 byte' }, + ]); + expect(settingsSubmissionOf(invalid)).toBeNull(); }); - it('keeps a write in flight when a fresh read lands underneath it', () => { - const inFlight = run( - confirmedProjectChange(), - { type: 'submit_started' }, - { type: 'authority_observed', authority: withProjectRevision(AUTHORITY, 'rev-43') }, - ); + it('preserves the exact request identity across an uncertain transport retry', () => { + const first = expectSubmitting(saveProjectChange()); + const original = settingsSubmission(first, ROUTES); + const failed = settle(first, { + outcome: 'offline', + detail: 'The response was lost after dispatch.', + }); + const retried = expectSubmitting(run(failed, { type: 'retry_requested' })); - expect(inFlight.status).toBe('submitting'); - // The request already named rev-42; the pre-flight check owns the verdict. - expect(settingsSubmission(expectSubmitting(inFlight), ROUTES).expectedRevisionId).toBe( - 'rev-42', - ); + expect(settingsSubmission(retried, ROUTES)).toEqual(original); }); - /** Leaving `submitting` would make `settleSubmit` drop the verdict when it - * lands: a conflict or refusal for scope A would vanish because the reader - * touched a scope-B field meanwhile. Edits and new reviews wait instead. */ - it('cannot be redrafted or re-reviewed out of a write that is already in flight', () => { - const submitting = run(confirmedProjectChange(), { type: 'submit_started' }); - expect(submitting.status).toBe('submitting'); - - const redrafted = run(submitting, { - type: 'user_drafted', - values: { ...AUTHORITY.user, watcher_debounce: '15s' }, + it('does not retry an uncertain effect and requires current values instead', () => { + const submitted = expectSubmitting(saveProjectChange()); + const uncertain = settle(submitted, { + outcome: 'unsettled', + detail: 'The write may have been applied.', }); - expect(redrafted).toBe(submitting); - const rereviewed = run(submitting, { - type: 'review_requested', - scope: 'user', + expect(uncertain.status).toBe('submit_failed'); + expect(run(uncertain, { type: 'retry_requested' })).toBe(uncertain); + expect(settingsSubmissionOf(uncertain)).toMatchObject({ idempotencyKey: IDEMPOTENCY_KEY, + patch: { max_file_size: 2_097_152 }, }); - expect(rereviewed).toBe(submitting); + const reloaded = run(uncertain, { type: 'reloaded_from_authority' }); + expect(reloaded.status).toBe('editing'); + expect(settingsScopeDirty(reloaded, 'project')).toBe(false); }); - it('cannot be dismissed out of a write that is already in flight', () => { - const submitting = run(confirmedProjectChange(), { type: 'submit_started' }); - - expect(run(submitting, { type: 'review_dismissed' })).toBe(submitting); - }); -}); - -describe('settings editor: each verdict is its own state', () => { - const submitting = () => run(confirmedProjectChange(), { type: 'submit_started' }); - - // A withdrawn authority means nothing was attempted; a failed write means - // something was and did not land. The old surface rendered both as red text - // from the same mutation result. - it('lands a withdrawn authority somewhere a failed write cannot reach', () => { - const withdrawn = settle(submitting(), { - outcome: 'unavailable', - detail: 'Nothing was applied: configuration authority is unavailable.', - }); - const failed = settle(submitting(), { - outcome: 'error', - detail: 'Settings update failed (HTTP 500).', + it('keeps a stale-revision refusal actionable without overwriting the draft', () => { + const conflicted = settle(expectSubmitting(saveProjectChange()), { + outcome: 'conflict', + expectedRevisionId: 'rev-42', + actualRevisionId: 'rev-43', }); - expect(withdrawn.status).toBe('authority_withdrawn'); - expect(withdrawn).toMatchObject({ - detail: 'Nothing was applied: configuration authority is unavailable.', + expect(conflicted.status).toBe('conflicted'); + expect(conflicted).toMatchObject({ + conflict: { expectedRevisionId: 'rev-42', actualRevisionId: 'rev-43' }, + draft: { project: { max_file_size: '2097152' } }, }); - expect(failed.status).toBe('submit_failed'); - expect(failed).toMatchObject({ failure: { kind: 'error' } }); - expect(withdrawn.status).not.toBe(failed.status); - // Neither is a save. - expect(settingsApplied(withdrawn)).toBeNull(); - expect(settingsApplied(failed)).toBeNull(); + expect(settingsApplied(conflicted)).toBeNull(); + expect(run(conflicted, { type: 'retry_requested' })).toBe(conflicted); + expect(settingsScopeDirty(conflicted, 'project')).toBe(true); }); - it('reuses the held caller key when a response-loss retry resubmits the same intent', () => { - const first = expectSubmitting(submitting()); - const failed = settle(first, { - outcome: 'offline', - detail: 'The response was lost after dispatch.', + it('keeps authorization refusals distinct and retries the same submission', () => { + const denied = settle(expectSubmitting(saveProjectChange()), { + outcome: 'not_dispatched', + detail: 'Nothing was sent. This scope is read-only.', + }); + expect(denied).toMatchObject({ + status: 'authority_withdrawn', + detail: 'Nothing was sent. This scope is read-only.', }); - const retried = expectSubmitting(run(failed, { type: 'submit_started' })); - expect(settingsSubmission(first, ROUTES).idempotencyKey).toBe(IDEMPOTENCY_KEY); - expect(settingsSubmission(retried, ROUTES).idempotencyKey).toBe(IDEMPOTENCY_KEY); + const retried = expectSubmitting(run(denied, { type: 'retry_requested' })); + expect(settingsSubmission(retried, ROUTES)).toMatchObject({ + idempotencyKey: IDEMPOTENCY_KEY, + expectedRevisionId: 'rev-42', + patch: { max_file_size: 2_097_152 }, + }); }); - it('keeps server validation as typed field state rather than a generic failure', () => { - const rejected = settle(submitting(), { + it('keeps server validation attached to the field until the user edits again', () => { + const rejected = settle(expectSubmitting(saveProjectChange()), { outcome: 'validation', detail: 'settings validation failed', errors: [ { field: 'max_file_size', message: 'max_file_size is denied by the active policy' }, ], }); - - expect(rejected.status).toBe('editing'); - expect(rejected.status).not.toBe('submit_failed'); expect(settingsRejection(rejected)).toEqual({ origin: 'server', scope: 'project', @@ -348,60 +209,18 @@ describe('settings editor: each verdict is its own state', () => { { field: 'max_file_size', message: 'max_file_size is denied by the active policy' }, ], }); - expect(settingsRejection(rejected)?.errors).toHaveLength(1); - expect(settingsApplied(rejected)).toBeNull(); - expect(settingsReviewOf(rejected)).toBeNull(); - }); + expect(settingsSubmissionOf(rejected)).toBeNull(); - it('distinguishes a refused revision from a superseded one', () => { - const conflicted = settle(submitting(), { - outcome: 'conflict', - expectedRevisionId: 'rev-42', - actualRevisionId: 'rev-43', - }); - - expect(conflicted.status).toBe('conflicted'); - expect(conflicted).toMatchObject({ - conflict: { expectedRevisionId: 'rev-42', actualRevisionId: 'rev-43' }, - }); - expect(settingsApplied(conflicted)).toBeNull(); - }); - - it('records a protocol violation against the authority that committed it', () => { - const failed = settle(submitting(), { - outcome: 'protocol_error', - authority: 'PATCH /api/settings/project', - detail: 'PATCH /api/settings/project violated the settings contract: expected JSON.', - }); - - expect(failed).toMatchObject({ - status: 'submit_failed', - failure: { kind: 'protocol_error', authority: 'PATCH /api/settings/project' }, - }); - }); - - it('ignores a verdict for a write this state did not issue', () => { - const editing = initialSettingsEditorState(AUTHORITY); - - const settled = settle(editing, { - outcome: 'success', - scope: 'project', - payload: fixturePayload(), - revisionId: 'rev-43', - resyncRecommended: false, - restartRecommended: false, - effectReceipt: null, - effectReconciliation: null, + const corrected = run(rejected, { + type: 'project_drafted', + values: draftMaxFileSize('4096'), }); - - expect(settled).toBe(editing); - expect(settingsApplied(settled)).toBeNull(); + expect(settingsRejection(corrected)).toBeNull(); + expect(settingsScopeDirty(corrected, 'project')).toBe(true); }); -}); -describe('settings editor: a save and a pending change cannot be shown at once', () => { - const applied = () => - settle(run(confirmedProjectChange(), { type: 'submit_started' }), { + it('records successful saves and their server-reported follow-up requirements', () => { + const saved = settle(expectSubmitting(saveProjectChange()), { outcome: 'success', scope: 'project', payload: fixturePayload(), @@ -412,181 +231,55 @@ describe('settings editor: a save and a pending change cannot be shown at once', effectReconciliation: null, }); - it('records the save the authority reported, and nothing it did not', () => { - const state = applied(); - - expect(state.status).toBe('editing'); - expect(settingsApplied(state)).toEqual({ - scope: 'project', + expect(settingsApplied(saved)).toMatchObject({ message: 'Project settings saved', revisionId: 'rev-43', resyncRecommended: true, restartRecommended: false, - effectReconciliation: null, - }); - expect(settingsRejection(state)).toBeNull(); - expect(settingsReviewOf(state)).toBeNull(); - }); - - // Previously: `notice` survived every subsequent review, so the editor could - // say "Project settings saved" directly above an unsent change. - it('drops the save notice as soon as another change goes under review', () => { - const reviewing = run( - applied(), - { type: 'project_drafted', values: draftMaxFileSize('4096') }, - { - type: 'review_requested', - scope: 'project', - idempotencyKey: IDEMPOTENCY_KEY, - }, - ); - - expect(reviewing.status).toBe('reviewing'); - expect(settingsApplied(reviewing)).toBeNull(); - }); - - it('keeps the save across the refetch it triggered', () => { - const refetched = run(applied(), { - type: 'authority_observed', - authority: withProjectRevision( - withProjectValues(AUTHORITY, { max_file_size: '2097152' }), - 'rev-43', - ), }); - - expect(settingsApplied(refetched)).toMatchObject({ message: 'Project settings saved' }); - expect(settingsScopeDirty(refetched, 'project')).toBe(false); + expect(settingsRejection(saved)).toBeNull(); + expect(settingsSubmissionOf(saved)).toBeNull(); }); - /** A refusal judged the draft as it was; once the draft moves the refusal - * describes nothing, and the live plan takes over. A completed write, by - * contrast, stays true across an edit. */ - it('drops a resting refusal on the next edit but keeps a completed write', () => { - const rejected = run(initialSettingsEditorState(AUTHORITY), { - type: 'project_drafted', - values: draftMaxFileSize('0'), - }, { - type: 'review_requested', - scope: 'project', - idempotencyKey: IDEMPOTENCY_KEY, - }); - expect(settingsRejection(rejected)).not.toBeNull(); - const retyped = run(rejected, { type: 'project_drafted', values: draftMaxFileSize('4096') }); - expect(settingsRejection(retyped)).toBeNull(); - expect(settingsScopeDirty(retyped, 'project')).toBe(true); - - const saved = applied(); - const edited = run(saved, { type: 'project_drafted', values: draftMaxFileSize('4096') }); - expect(settingsApplied(edited)).not.toBeNull(); - }); - - it('does not carry a refusal across a snapshot that replaced the values it judged', () => { - const rejected = run(initialSettingsEditorState(AUTHORITY), { - type: 'review_requested', - scope: 'project', - idempotencyKey: IDEMPOTENCY_KEY, - }); - expect(settingsRejection(rejected)).toMatchObject({ origin: 'client' }); - - const refetched = run(rejected, { + it('holds the submitted request and verdict while refreshes arrive', () => { + const submitted = expectSubmitting(saveProjectChange()); + const observed = run(submitted, { type: 'authority_observed', authority: withProjectRevision(AUTHORITY, 'rev-43'), }); - expect(settingsRejection(refetched)).toBeNull(); - }); - - it('closing the review does not undo a verdict the editor is resting on', () => { - const state = applied(); - - expect(run(state, { type: 'review_dismissed' })).toBe(state); - expect(settingsApplied(run(state, { type: 'review_dismissed' }))).not.toBeNull(); - }); -}); - -describe('settings editor: refusing to review what cannot be sent', () => { - it('refuses an unchanged draft as a stated rejection rather than an empty review', () => { - const state = run(initialSettingsEditorState(AUTHORITY), { - type: 'review_requested', - scope: 'user', - idempotencyKey: IDEMPOTENCY_KEY, - }); - - expect(settingsReviewOf(state)).toBeNull(); - expect(settingsRejection(state)).toEqual({ - origin: 'client', - scope: 'user', - errors: [{ field: 'user', message: 'No user settings have changed.' }], - }); - }); - - it('refuses values the daemon would reject, without building a review', () => { - const state = run( - initialSettingsEditorState(AUTHORITY), - { type: 'project_drafted', values: draftPollSecs('59') }, - { - type: 'review_requested', - scope: 'project', - idempotencyKey: IDEMPOTENCY_KEY, - }, + expect(observed).toBe(submitted); + expect(settingsSubmission(expectSubmitting(observed), ROUTES).expectedRevisionId).toBe( + 'rev-42', ); - - expect(settingsReviewOf(state)).toBeNull(); - expect(settingsRejection(state)?.errors).toEqual([ - { - field: 'auto_track_pr_poll_secs', - message: 'auto_track_pr_poll_secs must be at least 60 seconds', - }, - ]); + expect(run(observed, { + type: 'project_drafted', + values: draftMaxFileSize('4096'), + })).toBe(observed); }); - it('has no draft, review, or verdict at all while the read names no revision', () => { + it('does not dispatch a write without an editable authority', () => { const unavailable = initialSettingsEditorState(null); - - expect(unavailable).toEqual({ status: 'editor_unavailable' }); - expect(settingsReviewOf(unavailable)).toBeNull(); - expect(settingsApplied(unavailable)).toBeNull(); - expect(settingsRejection(unavailable)).toBeNull(); - expect(settingsScopeDirty(unavailable, 'project')).toBe(false); - for (const action of [ - { - type: 'review_requested', + expect( + run(unavailable, { + type: 'save_requested', scope: 'project', idempotencyKey: IDEMPOTENCY_KEY, - }, - { type: 'confirmation_set', confirmed: true }, - { type: 'submit_started' }, - ] satisfies SettingsEditorAction[]) { - expect(run(unavailable, action)).toBe(unavailable); - } - }); - - it('withdraws the whole editor when a later read names no revision', () => { - const withdrawn = run(confirmedProjectChange(), { - type: 'authority_observed', - authority: null, - }); - - expect(withdrawn).toEqual({ status: 'editor_unavailable' }); - }); - - it('leaves the draft alone when a read reports the snapshot it already holds', () => { - const drafted = run(initialSettingsEditorState(AUTHORITY), { - type: 'project_drafted', - values: draftMaxFileSize('2097152'), - }); - - const observed = run(drafted, { - type: 'authority_observed', - authority: fixtureAuthority(), - }); - - expect(observed).toBe(drafted); - expect(settingsScopeDirty(observed, 'project')).toBe(true); + }), + ).toBe(unavailable); + expect(settingsSubmissionOf(unavailable)).toBeNull(); }); }); -/* ----------------------------------------------------------------- setup --*/ +function saveProjectChange(): SubmittingSettingsState { + return expectSubmitting( + run( + initialSettingsEditorState(AUTHORITY), + { type: 'project_drafted', values: draftMaxFileSize('2097152') }, + { type: 'save_requested', scope: 'project', idempotencyKey: IDEMPOTENCY_KEY }, + ), + ); +} function run( state: SettingsEditorState, @@ -608,30 +301,10 @@ function expectSubmitting(state: SettingsEditorState): SubmittingSettingsState { return state; } -/** Narrowed, so a test can restate one field of it and still hold a real state. */ -function confirmedProjectChange(): Extract { - const state = run( - initialSettingsEditorState(AUTHORITY), - { type: 'project_drafted', values: draftMaxFileSize('2097152') }, - { - type: 'review_requested', - scope: 'project', - idempotencyKey: IDEMPOTENCY_KEY, - }, - { type: 'confirmation_set', confirmed: true }, - ); - if (state.status !== 'confirmed') throw new Error(`expected confirmed, got ${state.status}`); - return state; -} - function draftMaxFileSize(max_file_size: string): ProjectSettingsValues { return { ...AUTHORITY.project, max_file_size }; } -function draftPollSecs(auto_track_pr_poll_secs: string): ProjectSettingsValues { - return { ...AUTHORITY.project, auto_track_pr_poll_secs }; -} - function withProjectRevision( authority: SettingsEditor, projectExpectedRevisionId: string, @@ -639,14 +312,6 @@ function withProjectRevision( return { ...authority, projectExpectedRevisionId }; } -function withProjectValues( - authority: SettingsEditor, - values: Partial, -): SettingsEditor { - return { ...authority, project: { ...authority.project, ...values } }; -} - -/** The authority as the generated contract yields it from the served fixture. */ function fixtureAuthority(): SettingsEditor { const authority = buildSettingsEditor(fixturePayload()); if (!authority) throw new Error('the /api/settings fixture carries no editable authority'); diff --git a/dashboard/src/workspaces/settings/settingsEditorMachine.ts b/dashboard/src/workspaces/settings/settingsEditorMachine.ts index 270172e881..a2c9335d0b 100644 --- a/dashboard/src/workspaces/settings/settingsEditorMachine.ts +++ b/dashboard/src/workspaces/settings/settingsEditorMachine.ts @@ -1,23 +1,9 @@ /** * The settings editor as one state machine. * - * The editor used to be six independent `useState` slices, the project draft, - * the user draft, the pending review, a `confirmed` boolean, the validation - * errors, and the saved notice, that only added up to a correct screen when - * they happened to agree. Nothing stopped them disagreeing. A confirmation was - * a bare boolean beside the review it was supposedly given for, so a refetch - * could replace the draft underneath a confirmed review and the next apply - * would send a patch nobody had confirmed against a revision nobody was - * looking at. A saved notice sat beside a fresh pending change, telling the - * reader the change was already applied. - * - * Here those combinations do not exist to be reached. Every state names one - * situation and carries exactly what that situation has: only `editing` has - * rest state, only the review-bearing states have a review, and a confirmation - * is not a flag beside a review but a state that contains one. A review is - * identified by the exact change against the exact revision it was planned - * from (`reviewId`), so replanning is enough to tell whether the thing the - * user confirmed is still the thing about to be sent. + * One editable draft leads directly to a saved, rejected, or pending write. + * The exact patch, revision, and idempotency key are captured together only + * when Save is requested; retries reuse that submission unchanged. * * Everything here is pure. No DOM, no transport, no clock. */ @@ -26,11 +12,11 @@ import { planCodeIndexWorkerChangeAgainst, planProjectChangeAgainst, planUserChangeAgainst, + buildSettingsEditor, sameCodeIndexWorkerSelection, sameStrings, type CodeIndexWorkerSettingsChangeSet, type CodeIndexWorkerSettingsValues, - settingsRevisionId, type ProjectSettingsChangeSet, type ProjectSettingsValues, type SettingsChangePlan, @@ -52,25 +38,19 @@ export interface SettingsDraft { } /** - * A validated change frozen against the revision it was planned from. - * - * `reviewId` is the identity of that pairing. Two reviews with the same id are - * the same change against the same revision; anything else, an edited draft, - * a moved authority, produces a different id, which is how a confirmation - * given for one review is refused for another. + * One exact change held for submission and any idempotent retry. */ -interface SettingsReview { +export interface SettingsSubmission { readonly scope: SettingsScope; readonly expectedRevisionId: string; readonly patch: | ProjectSettingsChangeSet | UserSettingsChangeSet | CodeIndexWorkerSettingsChangeSet; - readonly reviewId: string; readonly idempotencyKey: string; } -/** A write the authority confirmed, as the authority reported it. */ +/** A write the authority applied, as the authority reported it. */ interface SettingsAppliedRecord { readonly scope: SettingsScope; readonly message: string; @@ -118,33 +98,26 @@ interface SettingsEditable { readonly draft: SettingsDraft; } -interface SettingsUnderReview extends SettingsEditable { - readonly review: SettingsReview; +interface SettingsWithSubmission extends SettingsEditable { + readonly submission: SettingsSubmission; } export type SettingsEditorState = /** The read parsed, but it named no revision the editor could hold a write against. */ | { readonly status: 'editor_unavailable' } | (SettingsEditable & { readonly status: 'editing'; readonly resting: SettingsResting }) - | (SettingsUnderReview & { readonly status: 'reviewing' }) - | (SettingsUnderReview & { readonly status: 'confirmed' }) - | (SettingsUnderReview & { readonly status: 'submitting' }) + | (SettingsWithSubmission & { readonly status: 'submitting' }) /** The write authority refused: it holds a different revision. Nothing was applied. */ - | (SettingsUnderReview & { + | (SettingsWithSubmission & { readonly status: 'conflicted'; readonly conflict: SettingsRevisionConflict; }) /** The authority for this write is not mounted. Nothing was attempted. */ - | (SettingsUnderReview & { readonly status: 'authority_withdrawn'; readonly detail: string }) + | (SettingsWithSubmission & { readonly status: 'authority_withdrawn'; readonly detail: string }) /** The write was attempted and did not reach a verdict. */ - | (SettingsUnderReview & { + | (SettingsWithSubmission & { readonly status: 'submit_failed'; readonly failure: SettingsSubmitFailure; - }) - /** The resource moved while this change was held. Nothing was attempted. */ - | (SettingsUnderReview & { - readonly status: 'review_superseded'; - readonly currentRevisionId: string; }); export type SettingsEditorAction = @@ -154,13 +127,11 @@ export type SettingsEditorAction = | { readonly type: 'user_drafted'; readonly values: UserSettingsValues } | { readonly type: 'code_index_workers_drafted'; readonly values: CodeIndexWorkerSettingsValues } | { - readonly type: 'review_requested'; + readonly type: 'save_requested'; readonly scope: SettingsScope; readonly idempotencyKey: string; } - | { readonly type: 'review_dismissed' } - | { readonly type: 'confirmation_set'; readonly confirmed: boolean } - | { readonly type: 'submit_started' } + | { readonly type: 'retry_requested' } | { readonly type: 'submit_settled'; readonly result: SettingsMutationResult } /** Discard the draft and take the authority's current values. */ | { readonly type: 'reloaded_from_authority' }; @@ -186,25 +157,14 @@ export function reduceSettingsEditor( return redraft(state, (draft) => ({ ...draft, user: action.values })); case 'code_index_workers_drafted': return redraft(state, (draft) => ({ ...draft, codeIndexWorkers: action.values })); - case 'review_requested': - return requestReview(state, action.scope, action.idempotencyKey); - case 'review_dismissed': - // A dismissal cannot cancel a write already in flight, and it cannot - // clear a verdict the editor is resting on, closing the dialog after a - // save must not take the save with it. - return state.status === 'editor_unavailable' || - state.status === 'editing' || - state.status === 'submitting' - ? state - : editing(state.authority, state.draft, CLEAN); - case 'confirmation_set': - return setConfirmation(state, action.confirmed); - case 'submit_started': - return startSubmit(state); + case 'save_requested': + return requestSave(state, action.scope, action.idempotencyKey); + case 'retry_requested': + return retrySubmit(state); case 'submit_settled': return settleSubmit(state, action.result); case 'reloaded_from_authority': - return state.status === 'editor_unavailable' + return state.status === 'editor_unavailable' || state.status === 'submitting' ? state : editing(state.authority, draftOf(state.authority), CLEAN); default: { @@ -217,46 +177,41 @@ export function reduceSettingsEditor( /** * A fresh read of the resource. * - * The draft always follows the authority, the editor shows what is there, not - * what was there. A review does not: it is a statement about a specific - * revision, so when the authority moves past that revision the review is - * superseded rather than quietly re-aimed at the new one. The one exception is - * a write already in flight, whose own pre-flight check owns that decision. + * A clean draft follows fresh values. An edited draft stays on the revision it + * came from so Save's preflight can report a conflict instead of silently + * replacing the user's work or re-aiming it at unseen values. */ function observeAuthority( state: SettingsEditorState, authority: SettingsEditor | null, ): SettingsEditorState { if (!authority) { - return state.status === 'editor_unavailable' ? state : EDITOR_UNAVAILABLE; + return state.status === 'editor_unavailable' || state.status !== 'editing' + ? state + : EDITOR_UNAVAILABLE; } if (state.status === 'editor_unavailable') { return editing(authority, draftOf(authority), CLEAN); } if (sameAuthority(state.authority, authority)) return state; - const draft = draftOf(authority); switch (state.status) { - case 'editing': - // A completed write stays true across the refetch it triggered; a - // refusal judged values the new snapshot has just replaced. + case 'editing': { + if ((['project', 'user', 'code_index_workers'] as const).some((scope) => + settingsScopeDirty(state, scope), + )) { + return state; + } return editing( authority, - draft, + draftOf(authority), state.resting.rest === 'applied' ? state.resting : CLEAN, ); - case 'submitting': - return { ...state, authority, draft }; - case 'reviewing': - case 'confirmed': + } case 'conflicted': case 'authority_withdrawn': case 'submit_failed': - case 'review_superseded': { - const current = settingsRevisionId(authority, state.review.scope); - return current === state.review.expectedRevisionId - ? { ...state, authority, draft } - : superseded(authority, draft, state.review, current); - } + case 'submitting': + return state; default: { const exhaustive: never = state; return exhaustive; @@ -265,38 +220,27 @@ function observeAuthority( } /** - * An edit. A review describes a draft; once the draft moves, the review - * describes nothing, so it goes, along with any confirmation given for it. - * - * A write in flight is the exception: leaving `submitting` would make - * `settleSubmit` discard the verdict when it lands, so the edit waits. A - * refusal the editor is resting on judged the draft as it was; once the draft - * moves it is no longer about anything and goes too, while a completed write - * stays true and stays. + * A write owns its immutable patch until it reaches a verdict. Editing becomes + * available again only after that verdict or an explicit reload. */ function redraft( state: SettingsEditorState, next: (draft: SettingsDraft) => SettingsDraft, ): SettingsEditorState { - if (state.status === 'editor_unavailable' || state.status === 'submitting') return state; - if (state.status === 'editing') { - return editing( - state.authority, - next(state.draft), - state.resting.rest === 'rejected' ? CLEAN : state.resting, - ); - } - return editing(state.authority, next(state.draft), CLEAN); + if (state.status !== 'editing') return state; + return editing( + state.authority, + next(state.draft), + state.resting.rest === 'rejected' ? CLEAN : state.resting, + ); } -function requestReview( +function requestSave( state: SettingsEditorState, scope: SettingsScope, idempotencyKey: string, ): SettingsEditorState { - // A review opened over a write in flight would replace the state that - // write's verdict is delivered to. It waits for the verdict. - if (state.status === 'editor_unavailable' || state.status === 'submitting') return state; + if (state.status !== 'editing') return state; const plan = planFor(state.authority, state.draft, scope); switch (plan.outcome) { case 'invalid': @@ -305,20 +249,18 @@ function requestReview( rejection: { origin: 'client', scope, errors: plan.errors }, }); case 'unchanged': - return editing(state.authority, state.draft, { - rest: 'rejected', - rejection: { - origin: 'client', - scope, - errors: [{ field: scope, message: `No ${scope} settings have changed.` }], - }, - }); + return state; case 'ready': return { - status: 'reviewing', + status: 'submitting', authority: state.authority, draft: state.draft, - review: reviewOf(scope, plan, idempotencyKey), + submission: { + scope, + expectedRevisionId: plan.expectedRevisionId, + patch: plan.patch, + idempotencyKey, + }, }; default: { const exhaustive: never = plan; @@ -327,81 +269,20 @@ function requestReview( } } -/** - * A confirmation is not a flag: it is the state that holds the review. So it - * can only be given while a review is open, and it can only be withdrawn from - * a state whose review it was given for. - */ -function setConfirmation( - state: SettingsEditorState, - confirmed: boolean, -): SettingsEditorState { - switch (state.status) { - case 'reviewing': - return confirmed - ? { - status: 'confirmed', - authority: state.authority, - draft: state.draft, - review: state.review, - } - : state; - case 'confirmed': - case 'authority_withdrawn': - case 'submit_failed': - return confirmed - ? state - : { - status: 'reviewing', - authority: state.authority, - draft: state.draft, - review: state.review, - }; - // A write in flight, a refused revision, and a superseded review are past - // the point where a checkbox decides anything. - case 'editor_unavailable': - case 'editing': - case 'submitting': - case 'conflicted': - case 'review_superseded': - return state; - default: { - const exhaustive: never = state; - return exhaustive; - } - } -} - -/** - * Entering the write. - * - * The confirmation is re-derived rather than trusted: the change is replanned - * against the authority the editor now holds, and only an identical review id - * lets the write proceed. The id carries the revision as well as the change, - * so one comparison catches both a resource that moved and a draft that no - * longer produces what was confirmed, either way the review is superseded - * instead of applied. - */ -function startSubmit(state: SettingsEditorState): SettingsEditorState { - // Reachable only from a confirmation: `submit_failed` and - // `authority_withdrawn` are themselves only reachable through one, so - // retrying from them is retrying a change the user did confirm. - if ( - state.status !== 'confirmed' && - state.status !== 'authority_withdrawn' && - state.status !== 'submit_failed' - ) { - return state; - } - const { authority, draft, review } = state; - const replanned = planFor(authority, draft, review.scope); +function retrySubmit(state: SettingsEditorState): SettingsEditorState { if ( - replanned.outcome !== 'ready' || - reviewOf(review.scope, replanned, review.idempotencyKey).reviewId !== review.reviewId + state.status === 'authority_withdrawn' || + (state.status === 'submit_failed' && + (state.failure.kind === 'offline' || state.failure.kind === 'error')) ) { - return superseded(authority, draft, review, settingsRevisionId(authority, review.scope)); + return { + status: 'submitting', + authority: state.authority, + draft: state.draft, + submission: state.submission, + }; } - return { status: 'submitting', authority, draft, review }; + return state; } function settleSubmit( @@ -409,10 +290,18 @@ function settleSubmit( result: SettingsMutationResult, ): SettingsEditorState { if (state.status !== 'submitting') return state; - const { authority, draft, review } = state; + const { authority, draft, submission } = state; switch (result.outcome) { case 'success': - return editing(authority, draft, { + const appliedAuthority = buildSettingsEditor(result.payload); + if (!appliedAuthority) { + return failed(state, { + kind: 'protocol_error', + authority: 'settings mutation', + detail: 'The settings response no longer carries editable values and revisions.', + }); + } + return editing(appliedAuthority, draftOf(appliedAuthority), { rest: 'applied', applied: { scope: result.scope, @@ -428,7 +317,7 @@ function settleSubmit( rest: 'rejected', rejection: { origin: 'server', - scope: review.scope, + scope: submission.scope, detail: result.detail, errors: result.errors, }, @@ -438,7 +327,7 @@ function settleSubmit( status: 'conflicted', authority, draft, - review, + submission, conflict: { expectedRevisionId: result.expectedRevisionId, actualRevisionId: result.actualRevisionId, @@ -451,7 +340,13 @@ function settleSubmit( case 'unavailable': case 'not_dispatched': case 'read_only_scope': - return { status: 'authority_withdrawn', authority, draft, review, detail: result.detail }; + return { + status: 'authority_withdrawn', + authority, + draft, + submission, + detail: result.detail, + }; case 'offline': return failed(state, { kind: 'offline', detail: result.detail }); case 'error': @@ -487,56 +382,31 @@ export type SubmittingSettingsState = Extract { return { - scope: state.review.scope, - expectedRevisionId: state.review.expectedRevisionId, - idempotencyKey: state.review.idempotencyKey, - readUrl: readUrlFor(state.review.scope, routes), - patchUrl: patchUrlFor(state.review.scope, routes), - patch: state.review.patch, + scope: state.submission.scope, + expectedRevisionId: state.submission.expectedRevisionId, + idempotencyKey: state.submission.idempotencyKey, + readUrl: readUrlFor(state.submission.scope, routes), + patchUrl: patchUrlFor(state.submission.scope, routes), + patch: state.submission.patch, }; } -/** The review the dialog is about, if the machine is holding one. */ -export function settingsReviewOf(state: SettingsEditorState): SettingsReview | null { +/** The immutable write awaiting a verdict or an explicit resolution. */ +export function settingsSubmissionOf(state: SettingsEditorState): SettingsSubmission | null { return state.status === 'editor_unavailable' || state.status === 'editing' ? null - : state.review; -} - -/** Whether the review on screen is one the user has confirmed. */ -export function settingsConfirmationHeld(state: SettingsEditorState): boolean { - switch (state.status) { - case 'confirmed': - case 'submitting': - case 'conflicted': - case 'authority_withdrawn': - case 'submit_failed': - return true; - case 'editor_unavailable': - case 'editing': - case 'reviewing': - case 'review_superseded': - return false; - default: { - const exhaustive: never = state; - return exhaustive; - } - } + : state.submission; } export function settingsRejection(state: SettingsEditorState): SettingsRejection | null { @@ -561,12 +431,8 @@ export function settingsScopeDirty( } /** - * The change this scope's draft currently amounts to, replanned live. - * - * This is the same plan `review_requested` freezes, computed without freezing - * it, so a surface can state validity, `ready`, `unchanged`, or `invalid` - * with the daemon-shaped field errors, while the value is still being typed. - * `null` while the read names no revision to plan against. + * The change this scope's draft currently amounts to, replanned live so the + * editor can state validity while the value is still being typed. */ export function settingsScopePlan( state: SettingsEditorState, @@ -588,15 +454,6 @@ function editing( return { status: 'editing', authority, draft, resting }; } -function superseded( - authority: SettingsEditor, - draft: SettingsDraft, - review: SettingsReview, - currentRevisionId: string, -): SettingsEditorState { - return { status: 'review_superseded', authority, draft, review, currentRevisionId }; -} - function failed( state: SubmittingSettingsState, failure: SettingsSubmitFailure, @@ -605,7 +462,7 @@ function failed( status: 'submit_failed', authority: state.authority, draft: state.draft, - review: state.review, + submission: state.submission, failure, }; } @@ -641,20 +498,6 @@ function planFor( } } -function reviewOf( - scope: SettingsScope, - plan: Extract, - idempotencyKey: string, -): SettingsReview { - return { - scope, - expectedRevisionId: plan.expectedRevisionId, - patch: plan.patch, - reviewId: `${scope}@${plan.expectedRevisionId}#${stableJson(plan.patch)}`, - idempotencyKey, - }; -} - function patchUrlFor(scope: SettingsScope, routes: SettingsRoutes): string { switch (scope) { case 'project': @@ -689,20 +532,6 @@ function savedMessage(scope: SettingsScope): string { } } -/** Key order never distinguishes two patches, so identity does not depend on it. */ -function stableJson(value: unknown): string { - if (Array.isArray(value)) { - return `[${value.map((item) => stableJson(item)).join(',')}]`; - } - if (typeof value === 'object' && value !== null) { - const entries = Object.entries(value as Record) - .sort(([left], [right]) => (left < right ? -1 : left > right ? 1 : 0)) - .map(([key, item]) => `${JSON.stringify(key)}:${stableJson(item)}`); - return `{${entries.join(',')}}`; - } - return JSON.stringify(value) ?? 'null'; -} - function sameAuthority(left: SettingsEditor, right: SettingsEditor): boolean { return ( left.projectExpectedRevisionId === right.projectExpectedRevisionId && diff --git a/dashboard/src/workspaces/settings/settingsRows.ts b/dashboard/src/workspaces/settings/settingsRows.ts index c96383879c..1739aec7f0 100644 --- a/dashboard/src/workspaces/settings/settingsRows.ts +++ b/dashboard/src/workspaces/settings/settingsRows.ts @@ -143,11 +143,11 @@ export function boundKeys(): readonly string[] { /** * What a row's write column states. Three answers, none of which is a * boolean: a key may be writable (and to what), locked by a gate (and by - * which, with the authority's reason), or simply have no write path at all. + * which, with the authority's reason), or simply cannot be edited here. * * `locked` carries a fourth gate beside the three scope gates: the read itself * named no revision to hold a write against, so the editor is unavailable for - * every bound key at once. Folded in here so the WRITE column and the review + * every bound key at once. Folded in here so the WRITE column and editor * panel cannot disagree about whether a key can be edited. */ export type WriteCapability = @@ -161,7 +161,7 @@ export type WriteCapability = | { readonly kind: 'no_write_path' }; const EDITOR_UNAVAILABLE_REASON = - 'GET /api/settings named no configuration revision to hold a write against, so nothing here can be edited until it does.'; + 'Current settings are unavailable, so editing is paused. Reload settings before trying again.'; export function writeCapability( key: string, @@ -181,7 +181,7 @@ export function writeCapability( return { kind: 'locked', gate: gate.state, - reason: `this dashboard is not authorized to apply ${scopeNoun(binding.scope)} settings`, + reason: `This dashboard cannot change ${scopeNoun(binding.scope)} settings. Ask an administrator for configuration-edit access.`, binding, }; case 'read_only': From 6a898e98b5ba2038e85f1c4fb812159e1b08938f Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Mon, 19 Oct 2026 04:30:56 +0000 Subject: [PATCH 3/4] fix(settings): clarify uncertain save outcomes Co-authored-by: ScriptedAlchemy <25274700+ScriptedAlchemy@users.noreply.github.com> --- .../settings/SettingsEditorPanel.tsx | 8 +++-- .../settings/SettingsPage.dom.test.tsx | 31 +++++++++++++++++-- .../src/workspaces/settings/settingsModel.ts | 6 ++-- .../settings/settingsMutation.test.ts | 17 ++++++++-- .../workspaces/settings/settingsMutation.ts | 18 +++++++++-- .../workspaces/settings/settingsRows.test.ts | 4 +-- 6 files changed, 69 insertions(+), 15 deletions(-) diff --git a/dashboard/src/workspaces/settings/SettingsEditorPanel.tsx b/dashboard/src/workspaces/settings/SettingsEditorPanel.tsx index 1f5298e162..402c83ad9c 100644 --- a/dashboard/src/workspaces/settings/SettingsEditorPanel.tsx +++ b/dashboard/src/workspaces/settings/SettingsEditorPanel.tsx @@ -58,7 +58,7 @@ export function SettingsEditorPanel({ }) { return (
@@ -409,7 +409,11 @@ function Verdict({ state }: { state: SettingsEditorState }) { return ( {state.failure.kind === 'unsettled' - ? `${state.failure.detail} Load current values before trying again.` + ? state.failure.detail + : state.failure.kind === 'protocol_error' + ? state.failure.authority.startsWith('PATCH ') + ? 'The save response could not be verified, so the change may have been applied. Load current values before trying again.' + : 'Current settings could not be verified, so nothing was saved. Load current values before trying again.' : state.failure.detail} ); diff --git a/dashboard/src/workspaces/settings/SettingsPage.dom.test.tsx b/dashboard/src/workspaces/settings/SettingsPage.dom.test.tsx index 1935e8eb13..fdd783f396 100644 --- a/dashboard/src/workspaces/settings/SettingsPage.dom.test.tsx +++ b/dashboard/src/workspaces/settings/SettingsPage.dom.test.tsx @@ -400,7 +400,7 @@ describe('SettingsPage direct settings editing', () => { await user.clear(proposal(MAX_FILE_SIZE)); await user.type(proposal(MAX_FILE_SIZE), '2097152'); await user.click(screen.getByRole('button', { name: 'Save' })); - expect(await screen.findByText(/Refresh current values before retrying/)).toBeTruthy(); + expect(await screen.findByText(/The settings change may have been applied/)).toBeTruthy(); expect(screen.queryByText('Project settings saved')).toBeNull(); expect(screen.queryByRole('button', { name: 'Retry project settings' })).toBeNull(); await user.click(screen.getByRole('button', { name: 'Load current values' })); @@ -409,6 +409,32 @@ describe('SettingsPage direct settings editing', () => { expect(screen.queryByText('Project settings saved')).toBeNull(); }); + it('requires readback after an incomplete PATCH response without repeating the write', async () => { + let applied = false; + const fetch = vi.fn(async (_input: RequestInfo | URL, init?: RequestInit) => { + if (init?.method === 'PATCH') { + applied = true; + return jsonResponse({}); + } + return jsonResponse(applied ? updatedSettings('rev-43') : settings()); + }); + vi.stubGlobal('fetch', fetch); + const user = userEvent.setup(); + renderSettings(); + + await openEditor(user, MAX_FILE_SIZE); + await user.clear(proposal(MAX_FILE_SIZE)); + await user.type(proposal(MAX_FILE_SIZE), '2097152'); + await user.click(screen.getByRole('button', { name: 'Save' })); + + expect(await screen.findByText(/the change may have been applied/)).toBeTruthy(); + expect(screen.queryByRole('button', { name: 'Retry project settings' })).toBeNull(); + await user.click(screen.getByRole('button', { name: 'Load current values' })); + await waitFor(() => expect(within(row(MAX_FILE_SIZE)).getByText('2,097,152')).toBeTruthy()); + expect(fetch.mock.calls.filter(([, init]) => init?.method === 'PATCH')).toHaveLength(1); + expect(screen.queryByText('Project settings saved')).toBeNull(); + }); + it('blocks a stale project change before sending the patch', async () => { let getCount = 0; const methods: string[] = []; @@ -646,6 +672,7 @@ describe('SettingsPage direct settings editing', () => { await screen.findByText(/Settings changed since you loaded them/); // Closing the editor does not hide the conflict from the inspector. + row(MAX_FILE_SIZE).focus(); await user.keyboard('{Escape}'); expect(document.querySelector('[data-settings-editor]')).toBeNull(); expect(document.querySelector('[data-settings-save-status="conflicted"]')?.textContent).toContain( @@ -1102,7 +1129,7 @@ describe('Settings response authority', () => { expect(row('storage.store_root').dataset['write']).toBe('no_write_path'); const panel = await openEditor(user, MAX_FILE_SIZE); expect(panel.querySelector('[data-settings-gate="editor_unavailable"]')?.textContent).toContain( - 'GET /api/settings named no configuration revision to hold a write against', + 'Current settings are unavailable, so editing is paused. Reload settings before trying again.', ); expect(screen.queryByLabelText(`Edit ${MAX_FILE_SIZE}`)).toBeNull(); // The inspector prints the empty revision as a stated absence. diff --git a/dashboard/src/workspaces/settings/settingsModel.ts b/dashboard/src/workspaces/settings/settingsModel.ts index 19cc9f3337..ff5a92f496 100644 --- a/dashboard/src/workspaces/settings/settingsModel.ts +++ b/dashboard/src/workspaces/settings/settingsModel.ts @@ -473,9 +473,8 @@ export function settingsRevisionId(current: SettingsEditor, scope: SettingsScope /** * The project change a draft represents against a known-current snapshot. * - * Planning against a snapshot rather than a payload is what lets the state - * machine replan without re-parsing: replanning is how a confirmation is - * checked against the review it was given for. + * Planning against a snapshot keeps edits tied to the values and revision they + * were based on; the resulting patch carries that revision to the server check. */ export function planProjectChangeAgainst( current: SettingsEditor | null, @@ -1018,4 +1017,3 @@ function humanize(key: string): string { const spaced = key.replace(/[_-]+/g, ' ').trim(); return spaced.charAt(0).toUpperCase() + spaced.slice(1); } - diff --git a/dashboard/src/workspaces/settings/settingsMutation.test.ts b/dashboard/src/workspaces/settings/settingsMutation.test.ts index fbae39842c..e65f91dc7c 100644 --- a/dashboard/src/workspaces/settings/settingsMutation.test.ts +++ b/dashboard/src/workspaces/settings/settingsMutation.test.ts @@ -70,7 +70,7 @@ describe('settings producer/consumer settlement', () => { ]) { const response = structuredClone(wire.changed); mutate(response.body.application_outcome.value); - expect(await apply(response)).toMatchObject({ outcome: 'unsettled', detail: expect.stringContaining('Refresh current values before retrying') }); + expect(await apply(response)).toMatchObject({ outcome: 'unsettled', detail: expect.stringContaining('The settings change may have been applied') }); } }); @@ -102,7 +102,20 @@ describe('settings producer/consumer settlement', () => { kind: 'partial_effect', committed_receipt: { ...wire.changed.body.application_outcome.value.receipt, outcome: 'partial' }, }); - expect(await apply(response)).toMatchObject({ outcome: 'unsettled', detail: expect.stringContaining('Refresh current values before retrying') }); + expect(await apply(response)).toMatchObject({ outcome: 'unsettled', detail: expect.stringContaining('The settings change may have been applied') }); + }); + + it('treats a lost PATCH response as uncertain and requires readback before another save', async () => { + const fetch = vi.fn() + .mockResolvedValueOnce(Response.json(wire.read.body, { status: wire.read.status })) + .mockRejectedValueOnce(new Error('connection lost')); + vi.stubGlobal('fetch', fetch); + + expect(await applySettingsMutation(request)).toEqual({ + outcome: 'unsettled', + detail: expect.stringContaining('The change may have been applied'), + }); + expect(fetch).toHaveBeenCalledTimes(2); }); it('rejects problem/status disagreement and malformed refusals', async () => { diff --git a/dashboard/src/workspaces/settings/settingsMutation.ts b/dashboard/src/workspaces/settings/settingsMutation.ts index 5c1a384274..23a63ded09 100644 --- a/dashboard/src/workspaces/settings/settingsMutation.ts +++ b/dashboard/src/workspaces/settings/settingsMutation.ts @@ -137,7 +137,13 @@ export async function applySettingsMutation( idempotency_key: request.idempotencyKey, }), }); - if (patched.outcome !== 'response') return patched; + if (patched.outcome === 'offline') { + return { + outcome: 'unsettled', + detail: 'The connection was lost while saving. The change may have been applied. Load current values before trying again.', + }; + } + if (patched.outcome === 'protocol_error') return patched; // The gateway's read-only refusal, recognized by its own body rather than by // the status alone: a 405 this dashboard cannot account for stays a plain // error below rather than borrowing the scope explanation. @@ -216,7 +222,10 @@ function projectSettingsPatchResponse( )) { return { parsed: false, - violation: { outcome: 'unsettled', detail: 'The settings effect is not confirmed. Refresh current values before retrying.' }, + violation: { + outcome: 'unsettled', + detail: 'The settings change may have been applied, but the result is uncertain. Load current values before trying again.', + }, }; } return { @@ -301,7 +310,10 @@ function settingsRefusal(body: unknown, status: number, authority: string): Sett : applicationProblemStatusMatches(status, refusal.problem.kind); if (!matches) return protocolError(authority, 'the application problem disagrees with its HTTP status.'); if (refusal.problem.committed_receipt !== null) { - return { outcome: 'unsettled', detail: `${refusal.problem.message} Refresh current values before retrying.` }; + return { + outcome: 'unsettled', + detail: `${refusal.problem.message} The settings change may have been applied. Load current values before trying again.`, + }; } return { outcome: 'error', detail: refusal.problem.message }; } diff --git a/dashboard/src/workspaces/settings/settingsRows.test.ts b/dashboard/src/workspaces/settings/settingsRows.test.ts index 3288aa2625..2548052032 100644 --- a/dashboard/src/workspaces/settings/settingsRows.test.ts +++ b/dashboard/src/workspaces/settings/settingsRows.test.ts @@ -99,12 +99,12 @@ describe('write bindings', () => { expect(writeCapability('project.config.include', unauthorized, true)).toMatchObject({ kind: 'locked', gate: 'unauthorized', - reason: 'this dashboard is not authorized to apply project settings', + reason: 'This dashboard cannot change project settings. Ask an administrator for configuration-edit access.', }); expect(writeCapability('user.code_index_workers', unauthorized, true)).toMatchObject({ kind: 'locked', gate: 'unauthorized', - reason: 'this dashboard is not authorized to apply code-index worker settings', + reason: 'This dashboard cannot change code-index worker settings. Ask an administrator for configuration-edit access.', }); const readOnly = writableScopes( From 9f8e9fba19db8fd73d467ad3e97c254cdb38b127 Mon Sep 17 00:00:00 2001 From: Zack Jackson <25274700+ScriptedAlchemy@users.noreply.github.com> Date: Tue, 20 Oct 2026 15:59:48 -0700 Subject: [PATCH 4/4] fix(settings): reload current authority before clearing conflicts --- .../settings/SettingsEditorController.tsx | 27 ++++++-- .../settings/SettingsPage.dom.test.tsx | 67 +++++++++++++++++++ .../src/workspaces/settings/SettingsPage.tsx | 15 +++-- .../settings/settingsEditorMachine.test.ts | 4 +- .../settings/settingsEditorMachine.ts | 6 +- 5 files changed, 103 insertions(+), 16 deletions(-) diff --git a/dashboard/src/workspaces/settings/SettingsEditorController.tsx b/dashboard/src/workspaces/settings/SettingsEditorController.tsx index 14fde11a31..e0c918e3c7 100644 --- a/dashboard/src/workspaces/settings/SettingsEditorController.tsx +++ b/dashboard/src/workspaces/settings/SettingsEditorController.tsx @@ -46,12 +46,12 @@ export function useSettingsEditor({ payload, routes, writability, - onApplied, + refresh, }: { payload: SettingsPayloadV1; routes: SettingsRoutes; writability: ScopeWritability; - onApplied: () => void; + refresh: () => Promise; }): SettingsEditorHandle { const authority = useMemo(() => buildSettingsEditor(payload), [payload]); const [state, dispatch] = useReducer( @@ -68,7 +68,7 @@ export function useSettingsEditor({ mutationFn: applySettingsMutation, onSuccess: (result) => { dispatch({ type: 'submit_settled', result }); - if (result.outcome === 'success') onApplied(); + if (result.outcome === 'success') void refresh(); }, onError: (error) => { dispatch({ @@ -129,10 +129,23 @@ export function useSettingsEditor({ }, []); const retry = useCallback(() => dispatch({ type: 'retry_requested' }), []); - const reload = useCallback(() => { - dispatch({ type: 'reloaded_from_authority' }); - onApplied(); - }, [onApplied]); + const reloadPending = useRef(false); + const reload = useCallback(async () => { + if (reloadPending.current) return; + reloadPending.current = true; + try { + const current = await refresh(); + const reloadedAuthority = current === null ? null : buildSettingsEditor(current); + // A failed or pending read cannot settle an uncertain write. Carry the + // successful read explicitly: React Query may reuse a snapshot already + // observed while the draft was dirty, so its effect need not run again. + if (reloadedAuthority !== null) { + dispatch({ type: 'reloaded_from_authority', authority: reloadedAuthority }); + } + } finally { + reloadPending.current = false; + } + }, [refresh]); return useMemo( () => ({ state, edit, revert, save, retry, reload }), diff --git a/dashboard/src/workspaces/settings/SettingsPage.dom.test.tsx b/dashboard/src/workspaces/settings/SettingsPage.dom.test.tsx index c2ebded98c..2a37dafa2b 100644 --- a/dashboard/src/workspaces/settings/SettingsPage.dom.test.tsx +++ b/dashboard/src/workspaces/settings/SettingsPage.dom.test.tsx @@ -476,6 +476,73 @@ describe('SettingsPage direct settings editing', () => { expect(methods).toEqual(['GET /api/settings', 'GET /api/settings', 'GET /api/settings']); }); + it('loads a conflict revision already observed while the draft was dirty', async () => { + let current = settings(); + const patches: Record[] = []; + vi.stubGlobal('fetch', vi.fn(async (input: RequestInfo | URL, init?: RequestInit) => { + const url = String(input); + if (url === '/api/settings/project' && init?.method === 'PATCH') { + patches.push(JSON.parse(String(init.body))); + current = updatedSettings('rev-44'); + return jsonResponse(projectPatchResponse(current)); + } + return jsonResponse(current); + })); + const user = userEvent.setup(); + const { client } = renderSettings(); + await openEditor(user, MAX_FILE_SIZE); + await user.clear(proposal(MAX_FILE_SIZE)); + await user.type(proposal(MAX_FILE_SIZE), '2097152'); + + current = updatedSettings('rev-43'); + const project = settingsBody(current)['project'] as Record; + (project['config'] as Record)['max_file_size'] = 4096; + await act(() => client.invalidateQueries({ queryKey: ['settings'] })); + await waitFor(() => expect(within(row(MAX_FILE_SIZE)).getByText('4,096')).toBeTruthy()); + expect((proposal(MAX_FILE_SIZE) as HTMLInputElement).value).toBe('2097152'); + + await user.click(screen.getByRole('button', { name: 'Save' })); + await screen.findByText(/Settings changed since you loaded them/); + await user.click(screen.getByRole('button', { name: 'Load current values' })); + expect(await screen.findByDisplayValue('4096')).toBeTruthy(); + + await user.clear(proposal(MAX_FILE_SIZE)); + await user.type(proposal(MAX_FILE_SIZE), '2097152'); + await user.click(screen.getByRole('button', { name: 'Save' })); + await screen.findByText('Project settings saved'); + expect(patches).toEqual([expect.objectContaining({ expected_revision_id: 'rev-43' })]); + }); + + it('keeps an uncertain write locked until its requested reload completes', async () => { + let patchAttempted = false; + let finishReload!: (response: Response) => void; + const reloadResponse = new Promise((resolve) => { finishReload = resolve; }); + vi.stubGlobal('fetch', vi.fn(async (input: RequestInfo | URL, init?: RequestInit) => { + const url = String(input); + if (url === '/api/settings/project' && init?.method === 'PATCH') { + patchAttempted = true; + throw new TypeError('connection lost'); + } + if (url === '/api/settings' && patchAttempted) return reloadResponse; + return jsonResponse(settings()); + })); + const user = userEvent.setup(); + renderSettings(); + await openEditor(user, MAX_FILE_SIZE); + await user.clear(proposal(MAX_FILE_SIZE)); + await user.type(proposal(MAX_FILE_SIZE), '2097152'); + await user.click(screen.getByRole('button', { name: 'Save' })); + await screen.findByText(/The connection was lost while saving/); + await user.click(screen.getByRole('button', { name: 'Load current values' })); + + expect(screen.getByText(/The connection was lost while saving/)).toBeTruthy(); + expect(screen.queryByRole('button', { name: 'Save' })).toBeNull(); + await act(async () => { finishReload(jsonResponse(updatedSettings('rev-43'))); }); + expect(await screen.findByDisplayValue('2097152')).toBeTruthy(); + expect(screen.queryByText(/The connection was lost while saving/)).toBeNull(); + expect(saveReadout()).toBe('none'); + }); + it('locks both configuration_batch scopes when the effect is withdrawn, and says why', async () => { const calls: string[] = []; vi.stubGlobal( diff --git a/dashboard/src/workspaces/settings/SettingsPage.tsx b/dashboard/src/workspaces/settings/SettingsPage.tsx index 770ac3c9a6..ac9af77b0e 100644 --- a/dashboard/src/workspaces/settings/SettingsPage.tsx +++ b/dashboard/src/workspaces/settings/SettingsPage.tsx @@ -74,7 +74,12 @@ export function SettingsPage() { [readUrl, scope], ); const refetch = settings.refetch; - const onApplied = useCallback(() => void refetch(), [refetch]); + const refresh = useCallback(async () => { + const result = await refetch(); + return !result.isError && result.data?.outcome === 'ok' + ? result.data.data.payload + : null; + }, [refetch]); return (
@@ -84,7 +89,7 @@ export function SettingsPage() { envelope={envelope} writability={writability} routes={routes} - onApplied={onApplied} + refresh={refresh} /> )} @@ -119,12 +124,12 @@ function SettingsSurface({ envelope, writability, routes, - onApplied, + refresh, }: { envelope: DashboardEnvelopeV1; writability: ScopeWritability; routes: SettingsRoutes; - onApplied: () => void; + refresh: () => Promise; }) { const payload = envelope.payload; const model = useMemo(() => buildSettingsModel(payload), [payload]); @@ -132,7 +137,7 @@ function SettingsSurface({ () => writableScopes(envelope.legal_actions, writability), [envelope.legal_actions, writability], ); - const editor = useSettingsEditor({ payload, routes, writability, onApplied }); + const editor = useSettingsEditor({ payload, routes, writability, refresh }); const [query, setQuery] = useState(''); const [inspectedKey, setInspectedKey] = useState(null); diff --git a/dashboard/src/workspaces/settings/settingsEditorMachine.test.ts b/dashboard/src/workspaces/settings/settingsEditorMachine.test.ts index 199636d3f8..12d0746425 100644 --- a/dashboard/src/workspaces/settings/settingsEditorMachine.test.ts +++ b/dashboard/src/workspaces/settings/settingsEditorMachine.test.ts @@ -153,9 +153,11 @@ describe('settings editor direct Save', () => { idempotencyKey: IDEMPOTENCY_KEY, patch: { max_file_size: 2_097_152 }, }); - const reloaded = run(uncertain, { type: 'reloaded_from_authority' }); + const authority = withProjectRevision(AUTHORITY, 'rev-43'); + const reloaded = run(uncertain, { type: 'reloaded_from_authority', authority }); expect(reloaded.status).toBe('editing'); expect(settingsScopeDirty(reloaded, 'project')).toBe(false); + expect(reloaded).toMatchObject({ authority: { projectExpectedRevisionId: 'rev-43' } }); }); it('keeps a stale-revision refusal actionable without overwriting the draft', () => { diff --git a/dashboard/src/workspaces/settings/settingsEditorMachine.ts b/dashboard/src/workspaces/settings/settingsEditorMachine.ts index 65c991ea88..465dcc838e 100644 --- a/dashboard/src/workspaces/settings/settingsEditorMachine.ts +++ b/dashboard/src/workspaces/settings/settingsEditorMachine.ts @@ -133,8 +133,8 @@ export type SettingsEditorAction = } | { readonly type: 'retry_requested' } | { readonly type: 'submit_settled'; readonly result: SettingsMutationResult } - /** Discard the draft and take the authority's current values. */ - | { readonly type: 'reloaded_from_authority' }; + /** Discard the draft only after a successful explicit authority read. */ + | { readonly type: 'reloaded_from_authority'; readonly authority: SettingsEditor }; const EDITOR_UNAVAILABLE: SettingsEditorState = { status: 'editor_unavailable' }; @@ -166,7 +166,7 @@ export function reduceSettingsEditor( case 'reloaded_from_authority': return state.status === 'editor_unavailable' || state.status === 'submitting' ? state - : editing(state.authority, draftOf(state.authority), CLEAN); + : editing(action.authority, draftOf(action.authority), CLEAN); default: { const exhaustive: never = action; return exhaustive;