diff --git a/docs/editor-buffer-corruption.md b/docs/editor-buffer-corruption.md index 2f8aeee5..ebb7ccb2 100644 --- a/docs/editor-buffer-corruption.md +++ b/docs/editor-buffer-corruption.md @@ -64,6 +64,38 @@ writes job output into `useEditorState` — `openFile`/`setContent`/`reloadFile` shape, and it is not proven to be the reported instance. **The trap is still the thing that names the text's source.** Do not delete it on the strength of this section. +## Fixed 2026-08-25: the buffer is shared, because the model already was + +`useEditorState` now keys buffers by path in one store shared across panels, and keeps only the tab set and +the active path per panel. The rule that matters: **`openFile` discards the content it is handed when the +file is already open somewhere else.** That mirrors what Monaco does with the model, and it is what removes +the divergence — with both panels reading one buffer there is never a `value` for the sync effect to +reconcile, so the `executeEdits` never runs and the cross-panel `onChange` never fires. + +A buffer outlives the tab that opened it. `holders` is the set of panels with the file open, and the entry +is dropped when the last one closes, so closing a tab in one panel cannot take unsaved work out from under +another. Closing the last one still discards unsaved edits, exactly as before. + +`` is the other half, and it was needed under any fix: the default disposes the +shared model on unmount, and `key={activeFile.path}` makes that every **tab switch**, not just a panel +close. The cost is one retained model per file opened in a session. + +Two things worth knowing: + +- **Writes go through `queryClient.setQueryData`, not `useGlobal`'s setter.** `useGlobal` applies a + functional update to the `data` captured in render rather than to what is in the cache now. With one + writer that is invisible; this slot has one writer per panel, which is the whole scenario. +- **The trap can now fire legitimately.** Two panels on one file share a model, so a large paste in one + fires the other's `onChange` while it is unfocused — which is what `reportSuspiciousEdit` looks for. A + report whose `stack` runs through the editor's own `onChange` with another panel open is that, not the + bug. This is the reason to keep reading the `stack` rather than the count. + +Pinned by `useEditorState.test.ts` (9 tests): adoption, cross-panel edit, close-with-holders, drop on last +close, release on unmount, and that the tab set stays per panel. + +**Still not closed.** This removes the mechanism, not the mystery: where the log text came from is still +unknown, and nothing found so far writes job output into an editor buffer. Keep the trap. + ## When it is reported Ask for one thing. In the browser's console, on the machine where it happened: diff --git a/src/workspaces/officerdev/src/apps/CodeEditor/CodeEditor.tsx b/src/workspaces/officerdev/src/apps/CodeEditor/CodeEditor.tsx index f8cb285b..8b51f55b 100644 --- a/src/workspaces/officerdev/src/apps/CodeEditor/CodeEditor.tsx +++ b/src/workspaces/officerdev/src/apps/CodeEditor/CodeEditor.tsx @@ -360,6 +360,17 @@ export const CodeEditorView = ({ // extension is what decides whether JSX is legal. Without it every tag in a `.tsx` file // was underlined as a syntax error. See `monaco-setup.ts`. path={activeFile.path} + /* + Do not dispose the model on unmount. It is shared — `path` keys it by file URI globally, + so two panels on one file hold the same model — and the default (`false`) disposes it + from under whoever else is using it. With `key={activeFile.path}` above, that unmount is + every TAB SWITCH, not just closing a panel. + + The cost is one retained model per file opened this session, which `useEditorState`'s + shared buffers now mirror exactly: the buffer for a file nobody has open is dropped, and + the model is not. Worth revisiting together if a long session ever shows it. + */ + keepCurrentModel theme={theme} language={getLanguage(activeFile.name)} value={activeFile.content} diff --git a/src/workspaces/officerdev/src/apps/CodeEditor/useEditorState.test.ts b/src/workspaces/officerdev/src/apps/CodeEditor/useEditorState.test.ts new file mode 100644 index 00000000..ea67a729 --- /dev/null +++ b/src/workspaces/officerdev/src/apps/CodeEditor/useEditorState.test.ts @@ -0,0 +1,178 @@ +import type { ReactNode } from 'react'; +import { createElement } from 'react'; +import { describe, expect, test } from 'bun:test'; +import { QueryClient, QueryClientProvider } from '@tanstack/react-query'; +import { MemoryRouter } from 'react-router'; +import { act, renderHook, waitFor } from '@testing-library/react'; +import { useEditorState, type OpenFile } from './useEditorState'; + +/** + * The buffer is shared across panels; the tab set is not. + * + * That split is forced by Monaco — `` keys its model by file URI globally, so two panels + * open on one file already share a model — and holding the content per panel on top of it is what produced + * `docs/editor-buffer-corruption.md`: one panel's programmatic write fired the OTHER panel's `onChange`, + * whose handler is `setContent`, so a buffer silently became the other panel's content and the tab went + * dirty with no keystroke. + * + * Two panels means two hook instances over one React Query cache, which is what `shared()` builds. + * + * Reads go through `waitFor`, not a bare assertion after `act`: the cross-panel re-render is a React Query + * observer notification, and `act` does not flush it here — which is the whole subject of these tests, so + * asserting synchronously would test nothing and pass. + */ +const shared = () => { + const client = new QueryClient({ defaultOptions: { queries: { retry: false } } }); + const wrapper = ({ children }: { children: ReactNode }) => + createElement(QueryClientProvider, { client }, createElement(MemoryRouter, null, children)); + return () => renderHook(() => useEditorState(), { wrapper }); +}; + +type Panel = ReturnType>; + +/** + * What this panel shows for a path, once every observer has settled. + * + * The expectation goes INSIDE `waitFor` deliberately. A helper that merely returns the file resolves on + * the first attempt and never retries, so the assertion outside it reads whatever was there before the + * other panel re-rendered — which passes for the wrong reason, or fails for a timing one. + */ +const expectFile = (panel: Panel, path: string, expected: Partial) => + waitFor(() => expect(panel.result.current.files.find((f) => f.path === path)).toMatchObject(expected)); + +const pathsIn = (panel: Panel, expected: string[]) => + waitFor(() => expect(panel.result.current.files.map((f) => f.path)).toEqual(expected)); + +describe('useEditorState — one buffer per file, shared by every panel', () => { + test('the second panel to open a file adopts the first one’s buffer', async () => { + const panel = shared(); + const a = panel(); + const b = panel(); + + act(() => a.result.current.openFile('/x.ts', 'x.ts', 'from disk')); + act(() => a.result.current.setContent('/x.ts', 'edited, unsaved')); + // B read the file itself and got what is on disk — which is now stale. The live buffer wins. + act(() => b.result.current.openFile('/x.ts', 'x.ts', 'from disk')); + + await expectFile(b, '/x.ts', { content: 'edited, unsaved', isDirty: true }); + }); + + test('an edit in one panel is the same edit in the other — there is no second copy to diverge', async () => { + const panel = shared(); + const a = panel(); + const b = panel(); + + act(() => a.result.current.openFile('/x.ts', 'x.ts', 'one')); + act(() => b.result.current.openFile('/x.ts', 'x.ts', 'one')); + act(() => b.result.current.setContent('/x.ts', 'two')); + + await expectFile(a, '/x.ts', { content: 'two' }); + await expectFile(b, '/x.ts', { content: 'two' }); + }); + + test('closing a tab in one panel leaves the other panel’s unsaved work alone', async () => { + const panel = shared(); + const a = panel(); + const b = panel(); + + act(() => a.result.current.openFile('/x.ts', 'x.ts', 'one')); + act(() => b.result.current.openFile('/x.ts', 'x.ts', 'one')); + act(() => b.result.current.setContent('/x.ts', 'unsaved')); + act(() => a.result.current.closeFile('/x.ts')); + + await pathsIn(a, []); + await expectFile(b, '/x.ts', { content: 'unsaved' }); + }); + + test('the buffer is dropped once the last panel holding it closes', async () => { + const panel = shared(); + const a = panel(); + const b = panel(); + + act(() => a.result.current.openFile('/x.ts', 'x.ts', 'one')); + act(() => b.result.current.openFile('/x.ts', 'x.ts', 'one')); + act(() => a.result.current.setContent('/x.ts', 'unsaved')); + act(() => a.result.current.closeFile('/x.ts')); + act(() => b.result.current.closeFile('/x.ts')); + await pathsIn(b, []); + + // Reopening reads the file again and gets the file — not a resurrected buffer. + act(() => a.result.current.openFile('/x.ts', 'x.ts', 'from disk')); + await expectFile(a, '/x.ts', { content: 'from disk' }); + }); + + test('a panel that goes away releases what it held', async () => { + const panel = shared(); + const a = panel(); + const b = panel(); + + act(() => a.result.current.openFile('/x.ts', 'x.ts', 'one')); + act(() => b.result.current.openFile('/x.ts', 'x.ts', 'one')); + act(() => b.unmount()); + act(() => a.result.current.closeFile('/x.ts')); + await pathsIn(a, []); + + act(() => a.result.current.openFile('/x.ts', 'x.ts', 'from disk')); + await expectFile(a, '/x.ts', { content: 'from disk' }); + }); + + test('the tab set stays per panel — sharing content is not sharing tabs', async () => { + const panel = shared(); + const a = panel(); + const b = panel(); + + act(() => a.result.current.openFile('/x.ts', 'x.ts', 'x')); + act(() => b.result.current.openFile('/y.ts', 'y.ts', 'y')); + + await pathsIn(a, ['/x.ts']); + await pathsIn(b, ['/y.ts']); + expect(a.result.current.activePath).toBe('/x.ts'); + expect(b.result.current.activePath).toBe('/y.ts'); + }); + + test('a file saved in one panel is clean in both', async () => { + const panel = shared(); + const a = panel(); + const b = panel(); + + act(() => a.result.current.openFile('/x.ts', 'x.ts', 'one')); + act(() => b.result.current.openFile('/x.ts', 'x.ts', 'one')); + act(() => a.result.current.setContent('/x.ts', 'two')); + await expectFile(b, '/x.ts', { isDirty: true }); + + act(() => a.result.current.markSaved('/x.ts', 'two')); + await expectFile(b, '/x.ts', { isDirty: false }); + }); + + test('reloading from disk clears dirty and missing for everyone holding it', async () => { + const panel = shared(); + const a = panel(); + const b = panel(); + + act(() => a.result.current.openFile('/x.ts', 'x.ts', 'one')); + act(() => b.result.current.openFile('/x.ts', 'x.ts', 'one')); + act(() => a.result.current.markMissing('/x.ts')); + await expectFile(b, '/x.ts', { missing: true, isDirty: true }); + + act(() => b.result.current.reloadFile('/x.ts', 'back')); + await expectFile(a, '/x.ts', { missing: false, isDirty: false, content: 'back' }); + }); + + test('opening the same file twice in ONE panel does not duplicate the tab or re-hold it', async () => { + const panel = shared(); + const a = panel(); + + act(() => a.result.current.openFile('/x.ts', 'x.ts', 'one')); + act(() => a.result.current.setContent('/x.ts', 'edited')); + act(() => a.result.current.openFile('/x.ts', 'x.ts', 'one')); + + await pathsIn(a, ['/x.ts']); + await expectFile(a, '/x.ts', { content: 'edited' }); + + // One hold, so one close is enough to drop it. + act(() => a.result.current.closeFile('/x.ts')); + await pathsIn(a, []); + act(() => a.result.current.openFile('/x.ts', 'x.ts', 'from disk')); + await expectFile(a, '/x.ts', { content: 'from disk' }); + }); +}); diff --git a/src/workspaces/officerdev/src/apps/CodeEditor/useEditorState.ts b/src/workspaces/officerdev/src/apps/CodeEditor/useEditorState.ts index 27f3b4ea..c51b31c1 100644 --- a/src/workspaces/officerdev/src/apps/CodeEditor/useEditorState.ts +++ b/src/workspaces/officerdev/src/apps/CodeEditor/useEditorState.ts @@ -1,5 +1,7 @@ -import { useState } from 'react'; +import { useEffect, useId, useMemo, useRef, useState } from 'react'; import { useSearchParams } from 'react-router'; +import { useQueryClient } from '@tanstack/react-query'; +import { globalQueryKey, useGlobal } from 'hooks/useGlobal'; export type OpenFile = { path: string; @@ -18,16 +20,89 @@ export type OpenFile = { */ export const EDITOR_FILE_PARAM = 'file'; +/** + * ── One buffer per file, shared by every panel showing it ── + * + * The tab SET is per panel; the CONTENT is not. That split is not a preference, it is forced by Monaco: + * `` keys its model by file URI **globally**, so two panels open on the same file already + * share one model whether we like it or not. + * + * Holding the content per panel on top of that shared model is what caused + * `docs/editor-buffer-corruption.md`. `@monaco-editor/react` looks a model up as + * `getModel(uri) || createModel(value)`, so the second panel to open a file adopts the first one's model + * and its own `value` is ignored; the value-sync effect then notices the mismatch and overwrites the + * shared model with `executeEdits`. The flag that suppresses `onChange` around that write is + * per-instance, so the OTHER panel's `onChange` fires — and the editor answers it with + * `setContent(path, …)`. One panel's buffer silently became the other's, and the tab went dirty with no + * keystroke. + * + * Sharing the state removes the mismatch that drives all of it: both panels read the same content, so + * there is never a `value` for the effect to reconcile. + * + * **The existing buffer wins.** `openFile` discards the content it was handed when the file is already + * open elsewhere, which mirrors what Monaco does with the model and is the whole point — the alternative + * is two answers for one buffer, which is the bug. + * + * A buffer outlives the tab that opened it. `holders` is the set of panels with it open, and the entry is + * dropped when the last one closes — so closing a tab in one panel cannot take unsaved work out from under + * another. Closing the LAST one still discards unsaved edits, exactly as it always did. + */ +type Buffer = Omit & { holders: string[] }; +type Buffers = Record; + +const EDITOR_BUFFERS_KEY = 'code-editor:buffers'; + /** * The *set* of open tabs is deliberately not in the URL. It is a working session, not an address — it * grows without bound, every entry costs a read on load, and nobody links someone else to a tab bar. * `file` is the selection, and a link to one opens it; the tabs accumulate around it as you browse. */ export const useEditorState = (urlState = false) => { - const [files, setFiles] = useState([]); + const panelKey = useId(); + const queryClient = useQueryClient(); + const [buffers] = useGlobal(EDITOR_BUFFERS_KEY, {}); + const [openPaths, setOpenPaths] = useState([]); const [localActive, setLocalActive] = useState(null); const [searchParams, setSearchParams] = useSearchParams(); + /* + Written through `setQueryData` rather than `useGlobal`'s setter, and that is load-bearing here. + + `useGlobal` applies a functional update to the `data` it captured in render, not to what is in the + cache now. With one writer that is invisible; this slot has one writer PER PANEL, which is the entire + scenario, and a stale base would drop whichever panel's edit landed second. `setQueryData` passes the + current value. + */ + const update = (fn: (prev: Buffers) => Buffers): void => { + queryClient.setQueryData(globalQueryKey(EDITOR_BUFFERS_KEY), (prev) => fn(prev ?? {})); + }; + + const patch = (path: string, fn: (buffer: Buffer) => Buffer): void => + update((prev) => { + const buffer = prev[path]; + return buffer ? { ...prev, [path]: fn(buffer) } : prev; + }); + + /** Let go of this panel's claim on some paths, dropping any buffer nobody is holding any more. */ + const release = (paths: string[]): void => + update((prev) => { + const next = { ...prev }; + for (const path of paths) { + const buffer = next[path]; + if (!buffer) continue; + const holders = buffer.holders.filter((holder) => holder !== panelKey); + if (holders.length) next[path] = { ...buffer, holders }; + else delete next[path]; + } + return next; + }); + + // A panel that goes away releases everything it held — otherwise a closed panel would pin every buffer + // it ever opened for the life of the tab. + const openPathsRef = useRef(openPaths); + openPathsRef.current = openPaths; + useEffect(() => () => release(openPathsRef.current), []); + const activePath = urlState ? searchParams.get(EDITOR_FILE_PARAM) : localActive; /** `replace` for a close — shutting a tab is a mutation, not somewhere you navigated to. */ @@ -56,9 +131,17 @@ export const useEditorState = (urlState = false) => { }; const openFile = (path: string, name: string, content: string) => { - setFiles((prev) => { - if (prev.some((f) => f.path === path)) return prev; - return [...prev, { path, name, content, originalContent: content, isDirty: false }]; + setOpenPaths((prev) => (prev.includes(path) ? prev : [...prev, path])); + update((prev) => { + const existing = prev[path]; + // Already open somewhere: take that buffer as it stands, `content` and all. See the note above — + // the copy on screen in the other panel may hold unsaved edits, and it is the same Monaco model. + if (existing) { + return existing.holders.includes(panelKey) + ? prev + : { ...prev, [path]: { ...existing, holders: [...existing.holders, panelKey] } }; + } + return { ...prev, [path]: { name, content, originalContent: content, isDirty: false, holders: [panelKey] } }; }); setActivePath(path); }; @@ -66,17 +149,15 @@ export const useEditorState = (urlState = false) => { // The next-active pick is computed here rather than inside the updater it used to sit in: a state // updater must be pure, and React invokes it twice in development precisely to catch this. const closeFile = (path: string) => { - const idx = files.findIndex((f) => f.path === path); - const next = files.filter((f) => f.path !== path); - setFiles(next); - if (activePath === path) setActivePath(next[Math.min(idx, next.length - 1)]?.path ?? null, true); + const idx = openPaths.indexOf(path); + const next = openPaths.filter((p) => p !== path); + setOpenPaths(next); + release([path]); + if (activePath === path) setActivePath(next[Math.min(idx, next.length - 1)] ?? null, true); }; - const setContent = (path: string, content: string) => { - setFiles((prev) => - prev.map((f) => (f.path === path ? { ...f, content, isDirty: content !== f.originalContent } : f)), - ); - }; + const setContent = (path: string, content: string) => + patch(path, (buffer) => ({ ...buffer, content, isDirty: content !== buffer.originalContent })); /** * Replace an open file's content with what is now on disk, leaving it CLEAN. @@ -85,13 +166,8 @@ export const useEditorState = (urlState = false) => { * which records that our own write landed. This is "someone else changed this file" — an agent turn, a * task, another editor — so both content and original move together and nothing is owed to disk. */ - const reloadFile = (path: string, content: string) => { - setFiles((prev) => - prev.map((f) => - f.path === path ? { ...f, content, originalContent: content, isDirty: false, missing: false } : f, - ), - ); - }; + const reloadFile = (path: string, content: string) => + patch(path, (buffer) => ({ ...buffer, content, originalContent: content, isDirty: false, missing: false })); /** * The file is open but no longer on disk — an agent deleted it, or a task moved it. @@ -104,22 +180,32 @@ export const useEditorState = (urlState = false) => { * disk (which is nothing), so Cmd+S writes it back and the file returns. Dirty also stops the refresh * loop retrying a read that will keep failing. */ - const markMissing = (path: string) => { - setFiles((prev) => prev.map((f) => (f.path === path ? { ...f, missing: true, isDirty: true } : f))); - }; + const markMissing = (path: string) => patch(path, (buffer) => ({ ...buffer, missing: true, isDirty: true })); /** * Our own write landed. Clears `missing` too: saving a deleted file PUTS IT BACK, so the tab must stop * saying it is gone — otherwise the one action that fixes the situation leaves the warning on screen * and reads as having failed. */ - const markSaved = (path: string, content: string) => { - setFiles((prev) => - prev.map((f) => - f.path === path ? { ...f, originalContent: content, content, isDirty: false, missing: false } : f, - ), - ); - }; + const markSaved = (path: string, content: string) => + patch(path, (buffer) => ({ ...buffer, content, originalContent: content, isDirty: false, missing: false })); + + /* + This panel's tabs, in its own order, resolved against the shared buffers. + + Memoised because it is a fresh array every render otherwise, and `CodeEditor` has an effect keyed on + `files` that persists the tab list — an unstable identity there writes panel memory on every render. + */ + const files = useMemo( + () => + openPaths.flatMap((path) => { + const buffer = buffers[path]; + if (!buffer) return []; + const { holders: _holders, ...rest } = buffer; + return [{ path, ...rest }]; + }), + [openPaths, buffers], + ); const getActiveFile = (): OpenFile | null => files.find((f) => f.path === activePath) ?? null; diff --git a/todo/known-bugs/background-job-logs-appeared-inside-an-open.md b/todo/known-bugs/background-job-logs-appeared-inside-an-open.md index 0e4d1a0b..62f89557 100644 --- a/todo/known-bugs/background-job-logs-appeared-inside-an-open.md +++ b/todo/known-bugs/background-job-logs-appeared-inside-an-open.md @@ -77,6 +77,20 @@ reopening. It does **not** explain how job output becomes one of those buffers, writes job output into `useEditorState`. So the trap stays armed; it is still the only thing that can name where the text came from. Full write-up in `docs/editor-buffer-corruption.md`. +**Mechanism removed 2026-08-25.** `useEditorState` now holds one buffer per file, shared by every panel +that has it open, and keeps only the tab set and active path per panel — the state now matches the model +Monaco was already sharing. `openFile` deliberately discards the content it is handed when the file is +open elsewhere, which is what removes the divergence: with both panels reading one buffer there is no +`value` for the sync effect to reconcile, so the `executeEdits` never runs and the cross-panel `onChange` +never fires. A buffer is refcounted by `holders` and dropped when the last panel closes it, so closing a +tab in one panel cannot discard another's unsaved work. `` stops the shared model +being disposed on every tab switch. Nine tests in `useEditorState.test.ts`. + +**Left open on purpose.** This removes a mechanism that produces the reported SHAPE; it does not explain +where the log text came from, and that question is the actual bug. The trap stays armed and +`docs/editor-buffer-corruption.md` stays. One new wrinkle recorded there: two panels on one file can now +fire the trap legitimately on a large paste, so the `stack` matters more than the count. + Worth noting because it is the reason to look at all: an editor that can show content the file does not contain is a correctness problem even when it never writes — the next person's instinct will be to save the file to "fix" it.