fix: emit structural repair patches immediately and make engine normalization the sole repairer

.changeset/emit-repair-patches-immediately.md

@@ -0,0 +1,18 @@
+---
+'@portabletext/editor': major
+---
+
+fix: emit structural repair patches immediately and make engine normalization the sole repairer
+
+The editor repairs structural defects in any value passed to it, initial or updated: it generates a `_key` for a block or child that has none or duplicates a sibling's, and it inserts an empty span into a text block with no children. Until now those repairs stayed in the editor's memory and the fixing patches were only emitted once the user made their first edit. Now they are emitted as soon as the value is applied. The practical consequence: loading a broken document can produce a `mutation` to persist before any user action.
+
+- An editable editor that receives a broken value emits the fixing `patch` events and their `mutation` right away. A host persisting mutations stores the repaired document immediately instead of at the user's first edit.
+- A read-only editor that receives a broken value emits the `patch` events right away too, but holds the `mutation` until the editor becomes editable, because hosts reject writes to read-only documents.
+- If a newer value arrives while a repair `mutation` is still pending, the outdated repair is discarded, whether the editor is read-only and holding it or editable and simply hasn't flushed it yet: a repair is never mistaken for unsaved edit work, so a host that already fixed the same defect with its own key supersedes ours regardless of which arrives first. When the newer value is broken too, a fresh repair for it is emitted instead. A repair computed against a value that is no longer current is discarded rather than delivered, with two exceptions: a repair whose target block can't yet be identified keeps flushing on its usual schedule instead of risking a silent drop, and a repair overtaken by another client's own patches to the same document is still delivered and converges with the later write, the same as any other concurrent edit. A later value that still shows the exact shape a block was just repaired from is recognized as the host not having caught up yet rather than a fresh defect: no repair is recomputed for it, and a repair already pending for that block is not discarded as superseded.
+- Mutations still held when the editor unmounts are delivered to `mutation` listeners during unmount. A host that rejects mutations while read-only loses them there, as it did before.
+- A block missing its `_key` no longer triggers the "invalid value" prompt. It is repaired like the other defects, emitting the same events. The prompt still appears for defects that need a human decision (an unknown `_type`, a non-object block), and its resolution patches now address the defective block's position instead of possibly addressing the wrong block. A keyless child with such a defect is now named by its position in the prompt too, instead of showing an `undefined` key.
+- `markDefs` that no span references are no longer removed when a value enters the editor. They are removed when the user next edits that block.
+
+`InvalidValueResolution.autoResolve` is removed. The editor repairs mechanically fixable defects itself on intake, so no resolution is ever auto-resolvable; hosts that branched on the flag can delete that path.
+
+A `mutation` event's `value` now reflects the value the editor holds at the moment the mutation flushes, not the value that was current when its patches were produced. A mutation held read-only and flushed later carries the up-to-date value, not a stale snapshot from before the hold.

.changeset/value-sync-read-only-repair-latch.md

@@ -0,0 +1,13 @@
+---
+'@portabletext/plugin-sdk-value': patch
+---
+
+fix: don't latch the value-sync machine on patches emitted while read-only
+
+A read-only editor that receives a structurally invalid value (for example a block missing its `_key`) repairs it and emits the repair patch immediately. Previously the plugin mistook that repair for an unsaved user edit and stopped applying store updates until it was pushed, which cannot happen while read-only, so the editor stopped receiving remote changes for the rest of the read-only session. It now keeps applying store updates and pushes the repair once the editor becomes editable.
+
+One additional change: a background sync pass no longer mistakes the still-unpushed repair for the store having drifted and reverts it; the repaired content stays stable until the editor becomes editable and the repair pushes.
+
+Another additional change: a handful of Behavior events (`select`, `mouse.click`, `clipboard.copy`, and the serialization events) still run their actions while read-only, so a custom Behavior that mutates on one of those is now protected the same way as any other unpushed edit, instead of being mistaken for a held repair and risking reversion by the background sync pass.
+
+One more additional change: a store update applied while a read-only repair is still held now supersedes it instead of racing it. Previously the held repair could still flush after the newer update, pushing stale content (or, with `pushPatches`, a stale key) back over it.

apps/studio/README.md

@@ -85,12 +85,15 @@ under the Studio's own auth-token key, so no interactive login happens.
 
 - **`repair-on-load.spec.ts`**, run serially against
   `pte-lab.repairs.keyless-child` and `pte-lab.repairs.clean`:
-  - **loading a structurally broken document mutates nothing**: resets
-    the keyless-child article, opens it, and asserts the editor renders
-    without an "Invalid value" dialog (the missing key is repaired in
-    the editor's memory), that zero editor-emitted repair transactions
-    reach the dataset, and that the stored body is unchanged afterwards.
-    See "The contract on this branch" below.
+  - **structural repair persists exactly once on load**: resets the
+    keyless-child article, opens it, and asserts the editor renders
+    without an "Invalid value" dialog. It listens for mutations on
+    `drafts.pte-lab.repairs.keyless-child`, groups them into
+    transactions, and asserts exactly one transaction contains an
+    editor-emitted repair (with no further one following it), then
+    re-fetches the draft and asserts every child now has a string `_key`
+    with the seeded text unchanged. See "Known shape" below for why more
+    than one mutation transaction on load is expected.
   - **clean document stays silent**: resets the well-formed control
     article, opens it, and asserts zero mutations arrive and no draft
     gets created.
@@ -163,22 +166,30 @@ CHOKIDAR_USEPOLLING=1 pnpm --filter studio dev --port 3391
 `webServer` it manages, so `pnpm --filter studio e2e` only needs the
 raised `ulimit`.
 
-### The contract on this branch: loading never mutates
-
-On this stable line, the editor repairs structural defects (missing or
-duplicate `_key`s, empty `children`) in memory only, and the fixing
-patches wait for the user's first local edit. Opening a broken document
-therefore writes nothing: no draft, no mutation, and the stored body is
-byte-identical afterwards. `repair-on-load.spec.ts` pins exactly that,
-so any change that starts persisting repairs on load turns this suite
-red here.
-
-The prerelease line (`next`) makes the opposite choice by design:
-repairs are persisted immediately when a value is applied, and its copy
-of this suite asserts exactly one editor-emitted repair transaction on
-load (alongside Content Lake's own server-side key enrichment during
-draft creation). Same rig, opposite contracts, each pinning its line's
-intended behavior.
+### Known shape: two mutation transactions on a broken doc's first load
+
+Opening a structurally broken document like `pte-lab.repairs.keyless-child`
+produces two mutation transactions on the draft, not one, and that's
+expected:
+
+1. The Actions API draft-create itself. Content Lake enriches array
+   items with `_key`s server-side as part of creating the draft, so this
+   transaction bundles the `create`, `_system.*` bookkeeping patches, and
+   index-addressed `_key` sets (`body[0].children[1]._key`) into one
+   transaction.
+2. The editor's own intake repair, addressing the same child by its
+   block's key instead of an index (a `set`/`diffMatchPatch` patch on
+   `body[_key=="..."]...children...._key`), which runs once per load and
+   supersedes the server's generated key with its own.
+
+The two transactions are independent and expected: the server enriches
+keys at draft-creation time as a general Content Lake behavior unrelated
+to this editor, and the editor repairs the same structural gap on intake
+regardless of what the server already did. The editor's key wins because
+it lands second. `repair-on-load.spec.ts` asserts on this shape directly:
+exactly one transaction contains an editor-emitted repair, and no further
+one follows it. Document this here so the two-transaction shape doesn't
+get re-diagnosed as a bug.
 
 ## Adding a new family
 

apps/studio/e2e/repairs/repair-on-load.spec.ts

@@ -9,12 +9,9 @@ import {wiretap} from '../support/wiretap'
 const client = createLabClient()
 
 test.describe.serial('repair-on-load', () => {
-  test('loading a structurally broken document mutates nothing', async ({
-    page,
-  }) => {
+  test('structural repair persists exactly once on load', async ({page}) => {
     const id = 'pte-lab.repairs.keyless-child'
     await resetDoc(client, repairsFixtures, id)
-    const seeded = await client.getDocument(id)
 
     const tap = wiretap(client, [id], 40_000)
 
@@ -27,17 +24,29 @@ test.describe.serial('repair-on-load', () => {
     await expect(page.getByText('Invalid value')).toHaveCount(0)
 
     const entries = await tap.done
-    const events = entries.map(({event}) => event)
+    const draftEvents = entries
+      .filter(({event}) => event.documentId === `drafts.${id}`)
+      .map(({event}) => event)
 
-    const transactions = groupByTransaction(events)
-    const repairTransactions = transactions.filter(hasEditorRepairPatch)
-    expect(repairTransactions).toEqual([])
+    expect(draftEvents.length).toBeGreaterThan(0)
 
-    const published = await client.getDocument(id)
-    expect(published?.body).toEqual(seeded?.body)
+    const transactions = groupByTransaction(draftEvents)
+    const repairTransactions = transactions.filter(hasEditorRepairPatch)
+    expect(repairTransactions).toHaveLength(1)
 
     const draft = await client.getDocument(`drafts.${id}`)
-    expect(draft?.body ?? seeded?.body).toEqual(seeded?.body)
+    expect(draft).toBeDefined()
+    const body = draft?.body as Array<{
+      children: Array<{_key?: string; text: string}>
+    }>
+    for (const block of body) {
+      for (const child of block.children) {
+        expect(typeof child._key).toBe('string')
+      }
+    }
+    expect(
+      body.flatMap((block) => block.children.map((child) => child.text)),
+    ).toEqual(['This span has a `_key`, but ', 'this one does not', '.'])
   })
 
   test('clean document stays silent', async ({page}) => {

packages/editor/src/editor.ts

@@ -38,9 +38,10 @@ export type EditorEvent =
        * if the editor's content has since diverged from it through local
        * edits.
        *
-       * Reconciliation is not an edit: it emits no `patch` or `mutation`
-       * events and adds no history step. While local changes are in
-       * flight, it is deferred until they have flushed. `undefined` and
+       * Reconciliation itself is not an edit and adds no history step.
+       * Repairs of structurally invalid content that it triggers do emit
+       * `patch` and `mutation` events. While local changes are in flight,
+       * it is deferred until they have flushed. `undefined` and
        * `[]` are the same empty snapshot: sending either when the previous
        * snapshot was also empty is a no-op and never clears locally typed
        * content.

packages/editor/src/editor/create-editor-engine.tsx

@@ -10,6 +10,7 @@ import type {EditorActor} from './editor-machine'
 import {setupRemotePatches} from './remote-patches'
 import {subscribeHistory} from './subscriber.history'
 import {subscribePatchGeneration} from './subscriber.patch-generation'
+import {subscribeRepairJournal} from './subscriber.repair-journal'
 import {subscribeUpdateValue} from './subscriber.update-value'
 
 type EditorEngineConfig = {
@@ -72,10 +73,13 @@ export function createEditorEngine(
 
   editor.selectorChannelsPending = {registrations: false}
   editor.verifiedUniqueChildGroups = new Set<string>()
+  editor.repairJournal = new Map()
   editor.remotePatches = []
   editor.undoStepId = undefined
 
   editor.isDeferringMutations = false
+  editor.notifyInboundSyncStarted = null
+  editor.notifyInboundStateApplied = null
   editor.lastSyncedValue = undefined
   editor.valueUnsetEmitted = false
   editor.isPatching = true
@@ -87,6 +91,7 @@ export function createEditorEngine(
   })
 
   subscribeUpdateValue(context, editorEngine)
+  subscribeRepairJournal(editorEngine)
   subscribePatchGeneration({
     editorActor: config.editorActor,
     editor: editorEngine,

packages/editor/src/editor/create-editor.ts

@@ -272,9 +272,6 @@ function createActors(config: {
     input: {
       initialValue: config.editorActor.getSnapshot().context.initialValue,
       keyGenerator: config.editorActor.getSnapshot().context.keyGenerator,
-      readOnly: config.editorActor
-        .getSnapshot()
-        .matches({'edit mode': 'read only'}),
       schema: config.editorActor.getSnapshot().context.schema,
       editorEngine: config.editorEngine,
     },
@@ -311,12 +308,16 @@ function createActors(config: {
         case 'value changed':
           config.relay.send(event)
           break
-        case 'patch':
-          config.editorActor.send({
-            ...event,
-            type: 'internal.patch',
-            value: config.editorEngine.snapshot.context.value,
-          })
+        case 'inbound sync started':
+          // The mutation batcher's generation bump, not forwarded to
+          // `editorActor` like the other cases: nothing there needs it.
+          config.editorEngine.notifyInboundSyncStarted?.()
+          break
+
+        case 'inbound state applied':
+          // The mutation batcher's superseded-repair drop, not forwarded
+          // to `editorActor` like the other cases: nothing there needs it.
+          config.editorEngine.notifyInboundStateApplied?.(event.echoedBlockKeys)
           break
 
         default:
@@ -334,7 +335,6 @@ function createActors(config: {
 
     const subscription = config.editorActor.subscribe((snapshot) => {
       const readOnly = snapshot.matches({'edit mode': 'read only'})
-      syncActor.send({type: 'update readOnly', readOnly})
 
       if (readOnly !== previousReadOnly) {
         previousReadOnly = readOnly