fix: one empty write and the editor could never save its tabs again
Reported not working, and the database said exactly why:
{"editor":{"p-1786832482923-77":{"open":[],"active":null}}}
The write worked. It wrote EMPTY, on the first mount before any file was open,
and from then on the panel was locked:
- an empty saved value is truthy
- the restore effect bailed on `open.length === 0` WITHOUT marking itself done
- the write guard was `if (restored && !didRestore.current) return`
so every later save was blocked by a restore that had already decided not to run.
Self-inflicted, and the guard was mine from two commits ago.
Underneath it was a second ordering bug that would have bitten even with the first
fixed: the restore is ASYNC — it reads each file — while the write effect runs on
the same mount, sees an empty `files`, and persists over the list it is in the
middle of reopening. Marking the restore done synchronously would just have moved
the failure.
Both are now one flag. `restoreDone` is state, set when the restore COMPLETES —
including when it completes by having nothing to do — and the write waits on it.
Nothing is persisted until the restore is genuinely finished.
Cleared the poisoned `editor` bag from the live row so the next test starts clean
rather than against state the old code had already wedged.
Worth naming the pattern: the guard conflated "there is saved state" with "a
restore is pending". Those differ exactly when the saved state is empty, which is
the state every panel starts in — so the bug was guaranteed rather than unlucky.
tsgo clean, frontend builds, 846 pass / 7 fail unchanged.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This commit is contained in:
@@ -98,49 +98,56 @@ export const CodeEditorView = ({
|
||||
}, [activePath, files]);
|
||||
|
||||
/**
|
||||
* Reopen what this panel had open before the reload.
|
||||
* Reopen what this panel had open before the reload, then start remembering.
|
||||
*
|
||||
* Sequential rather than parallel, because the order of the tabs is the order you left them in and
|
||||
* `openFile` appends. Each path is read fresh; one that no longer reads is skipped, so a file deleted
|
||||
* while the page was closed drops out of the strip instead of coming back as an empty buffer.
|
||||
* `restoreDone` is a state flag rather than a ref, and both halves depend on it, because the two ways
|
||||
* this goes wrong are both about ORDER:
|
||||
*
|
||||
* `active` is applied at the end and only if it actually opened — otherwise the last surviving tab wins,
|
||||
* which is what `closeFile` does when the active tab goes.
|
||||
* 1. Writing before the restore finishes clobbers the thing being restored. The restore is async — it
|
||||
* reads each file — while the write effect runs on the same mount, sees an empty `files`, and
|
||||
* persists `{open: [], active: null}` over the list it was about to reopen.
|
||||
*
|
||||
* 2. Treating "nothing to restore" as "restore still pending" locks the panel forever. That is the bug
|
||||
* this replaces: an empty saved value is TRUTHY, the restore bailed on `open.length === 0` without
|
||||
* marking itself done, and the write guard then blocked every subsequent save. One empty write and
|
||||
* the panel could never record anything again — which is exactly what the database showed.
|
||||
*
|
||||
* So the flag is set when the restore COMPLETES, including when it completes by having nothing to do.
|
||||
* Sequential opens, because tab order is the order you left them in and `openFile` appends.
|
||||
*/
|
||||
const didRestore = useRef(false);
|
||||
const [restoreDone, setRestoreDone] = useState(false);
|
||||
const startedRestore = useRef(false);
|
||||
|
||||
useEffect(() => {
|
||||
if (didRestore.current || !restored || restored.open.length === 0) return;
|
||||
didRestore.current = true;
|
||||
if (startedRestore.current) return;
|
||||
startedRestore.current = true;
|
||||
let alive = true;
|
||||
void (async () => {
|
||||
const wanted = restored?.open ?? [];
|
||||
const opened: string[] = [];
|
||||
for (const path of restored.open) {
|
||||
for (const path of wanted) {
|
||||
try {
|
||||
const res = await readFileRef.current(path);
|
||||
if (!alive) return;
|
||||
openFile(path, path.split('/').pop() ?? path, res.content);
|
||||
opened.push(path);
|
||||
} catch {
|
||||
// Gone or unreadable — leave it out rather than open an empty tab named after it.
|
||||
// Gone or unreadable — leave it out rather than open an empty tab named after a missing file.
|
||||
}
|
||||
}
|
||||
if (alive && restored.active && opened.includes(restored.active)) setActivePath(restored.active);
|
||||
if (!alive) return;
|
||||
if (restored?.active && opened.includes(restored.active)) setActivePath(restored.active);
|
||||
setRestoreDone(true);
|
||||
})();
|
||||
return () => {
|
||||
alive = false;
|
||||
};
|
||||
}, [restored]);
|
||||
|
||||
/**
|
||||
* Write the strip back whenever it changes.
|
||||
*
|
||||
* Guarded on the restore having run, or the first render — with no tabs open yet — would persist an
|
||||
* empty list over the very state it is about to read.
|
||||
*/
|
||||
useEffect(() => {
|
||||
if (restored && !didRestore.current) return;
|
||||
if (!restoreDone) return;
|
||||
remember({ open: files.map((f) => f.path), active: activePath });
|
||||
}, [files, activePath]);
|
||||
}, [files, activePath, restoreDone]);
|
||||
|
||||
/**
|
||||
* Re-read the disk: the tree, and every open file that has no unsaved edits.
|
||||
|
||||
Reference in New Issue
Block a user