vvzvlad/gitmost
1
0
Fork 0
Code Issues 14 Pull Requests 12 Packages Projects Releases Activity

fix(mcp): structural-diff write-back so agent edits don't jump the cursor (#152) #154

Merged
Ghost merged 3 commits from fix/mcp-comment-cursor-jump into develop 2026-06-24 14:49:47 +03:00
Conversation 2 Commits 3 Files Changed 11 +358 -51
Ghost commented 2026-06-24 05:57:05 +03:00
Copy Link

Закрывает #152.

Проблема

mutatePageContent (packages/mcp/src/lib/collaboration.ts) писал правки агента обратно через полную замену живого Yjs-документа: fragment.delete(0, len) + applyUpdate(новый Y.Doc). Yjs — CRDT, курсор редактора приклеен к id узлов через y-prosemirror. Полное удаление стирает все id → открытый редактор не может восстановить позицию → курсор прыгает в конец статьи. Заметнее всего на комментариях (текст не меняется, а курсор всё равно улетает). Регрессия с 4201f0a3 (до него привязка комментария молча проваливалась).

Решение

Запись через updateYFragment из y-prosemirror — ровно тот механизм, которым сам редактор синхронит правки в Yjs: структурный диф нового документа против живого фрагмента, трогаются только изменённые узлы, id неизменных сохраняются → курсор на месте.

Новый applyDocToFragment: sanitizeForYjs → PMNode.fromJSON против memoized docmost-схемы → updateYFragment в одном transact, с сохранением диагностики findUnstorableAttr.

Строгое улучшение для всех write-инструментов агента (правка текста, узлы, комментарии, replace): минимальный диф вместо полной замены — меньше шума в коллаборации, стабильные block-id, не сбрасываются чужие курсоры.

Тесты

  • comment-cursor-stability.test.mjs: Yjs RelativePosition (якорь курсора) переживает и правку соседнего узла, и привязку comment-mark (старая полная замена давала null).
  • 292 теста пакета зелёные (node --test).
  • y-prosemirror поднят в прямые зависимости (был транзитивный), пакет пересобран (build/ обновлён).

🤖 Generated with Claude Code

Закрывает #152. ## Проблема `mutatePageContent` (`packages/mcp/src/lib/collaboration.ts`) писал правки агента обратно через **полную замену** живого Yjs-документа: `fragment.delete(0, len)` + `applyUpdate(новый Y.Doc)`. Yjs — CRDT, курсор редактора приклеен к id узлов через y-prosemirror. Полное удаление стирает все id → открытый редактор не может восстановить позицию → курсор прыгает в конец статьи. Заметнее всего на **комментариях** (текст не меняется, а курсор всё равно улетает). Регрессия с `4201f0a3` (до него привязка комментария молча проваливалась). ## Решение Запись через `updateYFragment` из `y-prosemirror` — ровно тот механизм, которым сам редактор синхронит правки в Yjs: структурный диф нового документа против живого фрагмента, трогаются только изменённые узлы, id неизменных сохраняются → курсор на месте. Новый `applyDocToFragment`: `sanitizeForYjs` → `PMNode.fromJSON` против memoized docmost-схемы → `updateYFragment` в одном `transact`, с сохранением диагностики `findUnstorableAttr`. Строгое улучшение для **всех** write-инструментов агента (правка текста, узлы, комментарии, replace): минимальный диф вместо полной замены — меньше шума в коллаборации, стабильные block-id, не сбрасываются чужие курсоры. ## Тесты - `comment-cursor-stability.test.mjs`: Yjs `RelativePosition` (якорь курсора) переживает и правку соседнего узла, и привязку comment-mark (старая полная замена давала `null`). - **292 теста пакета зелёные** (`node --test`). - `y-prosemirror` поднят в прямые зависимости (был транзитивный), пакет пересобран (`build/` обновлён). 🤖 Generated with [Claude Code](https://claude.com/claude-code)
Ghost added 1 commit 2026-06-24 05:57:05 +03:00
fix(mcp): structural-diff write-back so agent edits don't jump the cursor (#152) f86b8b69a0 
mutatePageContent wrote agent edits back by DELETING the whole Yjs fragment and
re-applying a fresh Y.Doc. Yjs is a CRDT — the editor anchors its selection to
node ids — so wiping every id made an open editor's cursor lose its anchor and
snap to the end of the document on every agent write. It was most visible on
comment anchoring (issue #152): a comment changes no text, yet the cursor jumped.
(Before commit 4201f0a3 the anchoring silently no-op'd, so the destructive write
never ran for comments — hence the regression.)

Fix: write via `updateYFragment` (y-prosemirror) — the same routine the editor
uses to sync its own edits into Yjs. It structurally diffs the new doc against
the live fragment and touches only changed nodes, preserving the ids of unchanged
ones, so the cursor stays put. This improves ALL agent write tools (text edits,
node ops, comments, replace) — minimal diff instead of full replace: less collab
noise, stable block-ids, other users' cursors no longer disrupted.

- collaboration.ts: new `applyDocToFragment` (sanitize -> PMNode.fromJSON against
  a memoized docmost schema -> updateYFragment in one transact), keeping the
  `findUnstorableAttr` encode diagnostic; swap the destructive write-back for it.
- package.json: `y-prosemirror` promoted to a direct dependency (was transitive).
- test: comment-cursor-stability.test.mjs — a Yjs RelativePosition (the cursor
  anchor) survives both a sibling edit and a comment-mark anchoring (the old
  full-replace tombstoned it -> null). 292 package tests green.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Ghost added 1 commit 2026-06-24 12:56:38 +03:00
refactor(mcp): single docmostSchema + shared encode-error helper + catch test (#152 review) c7c0c28e38 
Review of #154 (Request changes) — all clean follow-ups, no defect in the fix:

1. Single source of the ProseMirror schema: export `docmostSchema` from
   docmost-schema.ts (next to docmostExtensions); diff.ts and collaboration.ts
   import it instead of each calling getSchema(docmostExtensions) — the schema
   can no longer drift between call sites. Removed both local builds + the now
   unused getSchema imports.
2. Doc fix: assertYjsEncodable's docstring and the client.ts comment no longer
   claim "the same encoder as apply" — apply uses updateYFragment, the dry-run
   uses toYdoc; both reject the same unstorable attrs but are NOT byte-identical.
   Reworded to "independent encodability gate".
3+4+5. Extracted `unstorableYjsError(safe, label, e)` — buildYDoc and
   applyDocToFragment now share one message template (label kept for diagnostics:
   toYdoc vs updateYFragment), so the wording can't drift between dry-run/apply.
6. Test for applyDocToFragment's catch branch: an unknown node type makes the
   schema-validated PMNode.fromJSON throw, and the function must re-throw it
   wrapped with the (updateYFragment) diagnostic.

build/ rebuilt for the three changed lib modules; 293 package tests green.
(Left build/client.js untouched: rebuilding it would pull in a pre-existing,
unrelated src/build drift — a listSidebarPages slugId fix never rebuilt on
develop — and my client.ts change there is comment-only.)

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
vvzvlad commented 2026-06-24 14:40:48 +03:00
Owner
Copy Link

Code review (multi-aspect) — структурный write-back против прыжка курсора (#152)

Вердикт: ✅ Approve with comments. Правка корректна, обоснована и покрыта тестами, которые реально фиксируют фикс (на до-фиксовом коде новые тесты падают). Все 280 unit-тестов пакета зелёные; персистентность подтверждена байт-в-байт идентичной старому пути. Все замечания ниже — уровня suggestion, ни одно не блокирует merge.

Ревью — 8 параллельных специализированных проходов (security, stability, conventions, documentation, regressions, test-coverage, simplification, architecture) по диапазону acf6d85b..c7c0c28e. Шум (закоммиченный build/, pnpm-симлинк node_modules/y-prosemirror, lock) — в рамках принятых в пакете конвенций.

Проверено и подтверждено

  • Переход «полная замена фрагмента → структурный диф через updateYFragment» корректен на граничных формах: пустой документ, документ→пусто, смена типа узла верхнего уровня, перестановка узлов, смена типа вложенного узла.
  • Сигнатура updateYFragment(ydoc, fragment, pmNode, { mapping, isOMark }) верна — четвёртый аргумент meta именно так конструируется внутри самой y-prosemirror 1.3.7.
  • Атомарность сохранена: ydoc.transact, синхронное окно read→write без await.
  • Тесты 1–2 действительно ловят регресс: на старом write-back обе RelativePosition резолвятся в null.
  • Security и simplification — чистый LGTM. Зависимость не растит supply-chain: y-prosemirror и его транзитивные пакеты уже были в дереве через @hocuspocus/transformer.

Замечания (suggestions, не блокеры)

  1. Диагностический label врёт про этап сбоя — packages/mcp/src/lib/collaboration.ts:15-22, 540-554.
    catch в applyDocToFragment оборачивает и PMNode.fromJSON (гидрация), и updateYFragment единым ярлыком (updateYFragment), хотя комментарий unstorableYjsError утверждает, что label «называет упавший этап». Новый тест сам это демонстрирует: неизвестный тип узла падает внутри PMNode.fromJSON — до updateYFragment, — а проверяется (updateYFragment). Влияния на поведение нет, только на точность диагностики.
    Фикс: либо смягчить комментарий (label = «путь кодирования», а не точный этап), либо разнести try, помечая сбой fromJSON ярлыком "fromJSON".

  2. Не покрыты структурные граничные случаи дифа — packages/mcp/test/unit/comment-cursor-stability.test.mjs.
    Тесты 1–2 покрывают правку текста сос��днего узла и добавление comment-mark, но не: очистку документа (content: []), удаление/добавление узла, смену типа узла верхнего уровня. Сейчас всё работает (проверено), но регресс именно в дифф-пути это не поймает.
    Фикс: добавить (a) неизменный абзац сохраняет id/курсор при удалении соседнего; (b) запись doc→пусто очищает фрагмент без исключения.

  3. Ветка «Offending attribute:» в unstorableYjsError не прогоняется — collaboration.ts:15-28.
    Тест 3 бьёт только в ветку bad == null. Через applyDocToFragment ветку с непустым findUnstorableAttr практически не достать (sanitizeForYjs срезает undefined раньше). Минорно — findUnstorableAttr отдельно протестирован в node-ops-table.test.mjs. Можно принять как есть.

  4. Фикс не распространён на путь замены изображения — packages/mcp/src/client.ts:489-491.
    mutateLiveContentUnlocked (используется replaceImage, см. client.ts:2659 и :2713) всё ещё пишет по-старому: fragment.delete(0, length) + Y.applyUpdate(encodeStateAsUpdate(tempDoc)). То есть для инструмента замены изображения прыжок курсора #152 сохраняется. Это предсуществующий код вне диффа PR — не регресс, но фикс неполон.
    Фикс: при желании перевести и этот путь на applyDocToFragment отдельным шагом, либо явно отметить в issue, что image-replace покрывается отдельно.

Архитектура (forward-looking, не блокирует merge)

A. Зависимость от приватного updateYFragment на основном write-пути.
updateYFragment помечен в y-prosemirror как @private/@unstable; публичные обёртки документированы только для пустого фрагмента, а здесь диф по живому — публичной альтернативы под задачу нет. Сигнал именно из этого репозитория: сервер (apps/server/src/collaboration/yjs.util.ts:51) намеренно обходит updateYFragment с комментарием «which has compatibility issues». Версия запинена точно (1.3.7), на этом пути нет ни одного теста с реальным Y.Doc.

  • A — закрепить (рекомендую, small): оставить прямой вызов + characterization-тест (fromYdoc → applyDocToFragment → fromYdoc на живом фрагменте с проверкой сохранения id) + однострочный комментарий, почему API внутренний и версия запинена.
  • B — вендорить рутину дифа (large): независимость от churn'а апстрима, но большой объём в собственность ради одного вызова.
  • C — принять как есть (small): ноль работы, пин защищает, но один pnpm up от тихого регресса без теста-стража.

B. Расхождение preview/apply (три маршрута кодирования).
assertYjsEncodable (dry-run) валидирует через toYdoc, а реальный apply — через updateYFragment: превью использует другой кодер. Сам PR это честно отмечает («encodability gate, не репетиция apply»). Риск узкий (общий sanitizeForYjs + схема ловят доминирующий класс ошибок), реален лишь для документа, падающего только на PMNode.fromJSON или на живом дифе.

  • A — полная репетиция (medium): гонять applyDocToFragment против клона живого Y.Doc.
  • B — дешёвое усиление (small): добавить PMNode.fromJSON(docmostSchema, sanitizeForYjs(doc)) в гейт.
  • C — статус-кво (small): уже честно задокументировано.
  • Рекомендация: B или C — на усмотрение автора; полная репетиция избыточна для этой правки.

🤖 Multi-aspect review через code-review-orchestrator (8 проходов + judge-pass: дедуп, ре-категоризация, фильтр шума, сверка с исходниками).

## Code review (multi-aspect) — структурный write-back против прыжка курсора (#152) **Вердикт: ✅ Approve with comments.** Правка корректна, обоснована и покрыта тестами, которые реально фиксируют фикс (на до-фиксовом коде новые тесты падают). Все 280 unit-тестов пакета зелёные; персистентность подтверждена байт-в-байт идентичной старому пути. Все замечания ниже — уровня *suggestion*, ни одно не блокирует merge. Ревью — 8 параллельных специализированных проходов (security, stability, conventions, documentation, regressions, test-coverage, simplification, architecture) по диапазону `acf6d85b..c7c0c28e`. Шум (закоммиченный `build/`, pnpm-симлинк `node_modules/y-prosemirror`, lock) — в рамках принятых в пакете конвенций. ### Проверено и подтверждено - Переход «полная замена фрагмента → структурный диф через `updateYFragment`» корректен на граничных формах: пустой документ, документ→пусто, смена типа узла верхнего уровня, перестановка узлов, смена типа вложенного узла. - Сигнатура `updateYFragment(ydoc, fragment, pmNode, { mapping, isOMark })` **верна** — четвёртый аргумент `meta` именно так конструируется внутри самой y-prosemirror 1.3.7. - Атомарность сохранена: `ydoc.transact`, синхронное окно read→write без `await`. - Тесты 1–2 действительно ловят регресс: на старом write-back обе `RelativePosition` резолвятся в `null`. - Security и simplification — чистый LGTM. Зависимость не растит supply-chain: `y-prosemirror` и его транзитивные пакеты уже были в дереве через `@hocuspocus/transformer`. ### Замечания (suggestions, не блокеры) 1. **Диагностический label врёт про этап сбоя** — `packages/mcp/src/lib/collaboration.ts:15-22, 540-554`. `catch` в `applyDocToFragment` оборачивает и `PMNode.fromJSON` (гидрация), и `updateYFragment` единым ярлыком `(updateYFragment)`, хотя комментарий `unstorableYjsError` утверждает, что `label` «называет упавший этап». Новый тест сам это демонстрирует: неизвестный тип узла падает внутри `PMNode.fromJSON` — до `updateYFragment`, — а проверяется `(updateYFragment)`. Влияния на поведение нет, только на точность диагностики. *Фикс:* либо смягчить комментарий (label = «путь кодирования», а не точный этап), либо разнести `try`, помечая сбой `fromJSON` ярлыком `"fromJSON"`. 2. **Не покрыты структурные граничные случаи дифа** — `packages/mcp/test/unit/comment-cursor-stability.test.mjs`. Тесты 1–2 покрывают правку текста сос��днего узла и добавление comment-mark, но не: очистку документа (`content: []`), удаление/добавление узла, смену типа узла верхнего уровня. Сейчас всё работает (проверено), но регресс именно в дифф-пути это не поймает. *Фикс:* добавить (a) неизменный абзац сохраняет id/курсор при удалении соседнего; (b) запись `doc→пусто` очищает фрагмент без исключения. 3. **Ветка «Offending attribute:» в `unstorableYjsError` не прогоняется** — `collaboration.ts:15-28`. Тест 3 бьёт только в ветку `bad == null`. Через `applyDocToFragment` ветку с непустым `findUnstorableAttr` практически не достать (`sanitizeForYjs` срезает `undefined` раньше). Минорно — `findUnstorableAttr` отдельно протестирован в `node-ops-table.test.mjs`. Можно принять как есть. 4. **Фикс не распространён на путь замены изображения** — `packages/mcp/src/client.ts:489-491`. `mutateLiveContentUnlocked` (используется `replaceImage`, см. `client.ts:2659` и `:2713`) всё ещё пишет по-старому: `fragment.delete(0, length)` + `Y.applyUpdate(encodeStateAsUpdate(tempDoc))`. То есть для инструмента замены изображения прыжок курсора #152 сохраняется. Это предсуществующий код вне диффа PR — не регресс, но фикс неполон. *Фикс:* при желании перевести и этот путь на `applyDocToFragment` отдельным шагом, либо явно отметить в issue, что image-replace покрывается отдельно. ### Архитектура (forward-looking, не блокирует merge) **A. Зависимость от приватного `updateYFragment` на основном write-пути.** `updateYFragment` помечен в y-prosemirror как `@private`/`@unstable`; публичные обёртки документированы только для **пустого** фрагмента, а здесь диф по **живому** — публичной альтернативы под задачу нет. Сигнал именно из этого репозитория: сервер (`apps/server/src/collaboration/yjs.util.ts:51`) намеренно **обходит** `updateYFragment` с комментарием «which has compatibility issues». Версия запинена точно (`1.3.7`), на этом пути нет ни одного теста с реальным Y.Doc. - *A — закрепить (рекомендую, small):* оставить прямой вызов + characterization-тест (`fromYdoc → applyDocToFragment → fromYdoc` на живом фрагменте с проверкой сохранения id) + однострочный комментарий, почему API внутренний и версия запинена. - *B — вендорить рутину дифа (large):* независимость от churn'а апстрима, но большой объём в собственность ради одного вызова. - *C — принять как есть (small):* ноль работы, пин защищает, но один `pnpm up` от тихого регресса без теста-стража. **B. Расхождение preview/apply (три маршрута кодирования).** `assertYjsEncodable` (dry-run) валидирует через `toYdoc`, а реальный apply — через `updateYFragment`: превью использует **другой** кодер. Сам PR это честно отмечает («encodability gate, не репетиция apply»). Риск узкий (общий `sanitizeForYjs` + схема ловят доминирующий класс ошибок), реален лишь для документа, падающего только на `PMNode.fromJSON` или на живом дифе. - *A — полная репетиция (medium):* гонять `applyDocToFragment` против клона живого Y.Doc. - *B — дешёвое усиление (small):* добавить `PMNode.fromJSON(docmostSchema, sanitizeForYjs(doc))` в гейт. - *C — статус-кво (small):* уже честно задокументировано. - *Рекомендация:* B или C — на усмотрение автора; полная репетиция избыточна для этой правки. --- 🤖 Multi-aspect review через code-review-orchestrator (8 проходов + judge-pass: дедуп, ре-категоризация, фильтр шума, сверка с исходниками).
vvzvlad commented 2026-06-24 14:44:43 +03:00
Owner
Copy Link

после исправления можно мержить

после исправления можно мержить
Ghost added 1 commit 2026-06-24 14:49:12 +03:00
refactor(mcp): accurate encode-failure labels + diff edge-case tests (#154 review) aca075108c 
Addresses the approve-with-comments review on PR #154:

- applyDocToFragment: hydrate PMNode.fromJSON in its OWN try so a hydration
  failure (e.g. an unknown node type) is labelled "fromJSON" — the stage that
  actually threw — instead of the misleading "updateYFragment". The diagnostic
  comment on unstorableYjsError ("label names the stage that failed") is now
  truthful.
- assertYjsEncodable: also rehearse PMNode.fromJSON(docmostSchema, …) so a doc
  that would only fail in apply's hydration step is rejected at preview time too,
  narrowing the preview/apply gap (review suggestion B). Still cheap — no live
  fragment, no updateYFragment.
- Tests: relabel the diagnostic test to (fromJSON); add structural-diff edge
  cases — neighbour deletion keeps the unchanged node's cursor anchor, doc->empty
  clears the fragment without throwing, top-level node-type change diffs in
  place — plus a preview-gate test for the new fromJSON rehearsal. 297/297 green.

build/ rebuilt for the changed lib module only (build/client.js left untouched
to avoid pulling in pre-existing unrelated src/build drift).

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Ghost merged commit 6566d2153c into develop 2026-06-24 14:49:47 +03:00
Ghost deleted branch fix/mcp-comment-cursor-jump 2026-06-24 14:49:47 +03:00
Sign in to join this conversation.
Reviewers
No Reviewers
Labels
Clear labels
bug
Something isn't working
 documentation
Improvements or additions to documentation
 duplicate
This issue or pull request already exists
 enhancement
New feature or request
 epic
Large multi-phase effort spanning many changes
 feature
New functionality request
 good first issue
Good for newcomers
 help wanted
Extra attention is needed
 idea
Idea / proposal for discussion
 invalid
This doesn't seem right
 needs-human
эскалация: нужно решение человека
 question
Further information is requested
 refactor
Code cleanup / refactoring
 review/approved
в последнем ревью нет открытых blocking-находок
 review/changes-requested
последнее ревью оставило открытые blocking-находки
 review/needs
head не ревьюился (head != reviewed_head)
 security
Security / hardening issue
 status/blocked
ждёт зависимость blocked_by
 status/done
закрыто и проверено
 status/in-progress
в активной работе (мягкая заявка)
 status/ready
специфицировано, не заблокировано, ждёт исполнителя
 test
Test coverage / test infrastructure
 wontfix
This will not be worked on
No Label
Milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
agent_coder agent_reviewer vvzvlad
No Assignees
2 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: vvzvlad/gitmost#154
Delete Branch "fix/mcp-comment-cursor-jump"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?