From 418ec1886862d28dd4c3710c1379a8b8e8078483 Mon Sep 17 00:00:00 2001 From: JannikStreek Date: Tue, 17 Feb 2026 20:17:38 +0100 Subject: [PATCH 1/8] fix yjs delete bug with descendants (#1162) --- .../map-sync/map-sync.service.spec.ts | 99 +++++++++++++++++++ .../services/map-sync/map-sync.service.ts | 12 ++- .../app/core/services/map-sync/yjs-utils.ts | 38 +++++++ 3 files changed, 148 insertions(+), 1 deletion(-) diff --git a/teammapper-frontend/src/app/core/services/map-sync/map-sync.service.spec.ts b/teammapper-frontend/src/app/core/services/map-sync/map-sync.service.spec.ts index 3e616d1b..c219b4ae 100644 --- a/teammapper-frontend/src/app/core/services/map-sync/map-sync.service.spec.ts +++ b/teammapper-frontend/src/app/core/services/map-sync/map-sync.service.spec.ts @@ -29,6 +29,7 @@ import { resolveMmpPropertyUpdate, resolveCompoundMmpUpdates, sortParentFirst, + collectDescendantIds, } from './yjs-utils'; // Mock the NodePropertyMapping module @@ -833,3 +834,101 @@ describe('sortParentFirst', () => { expect(result.map(n => n.id)).toEqual(['root', 'child', 'orphan']); }); }); + +describe('collectDescendantIds', () => { + let doc: Y.Doc; + let nodesMap: Y.Map>; + + function addNode(id: string, parent: string | null): void { + const yNode = new Y.Map(); + populateYMapFromNodeProps( + yNode, + createMockNode({ id, parent, isRoot: !parent }) + ); + nodesMap.set(id, yNode); + } + + beforeEach(() => { + doc = new Y.Doc(); + nodesMap = doc.getMap('nodes') as Y.Map>; + }); + + afterEach(() => { + doc.destroy(); + }); + + it('collects direct children of deleted node', () => { + addNode('root', null); + addNode('A', 'root'); + addNode('B', 'A'); + + const result = collectDescendantIds(nodesMap, 'A'); + + expect(result).toEqual(['B']); + }); + + it('collects all nested descendants (A=>B=>C, delete A)', () => { + addNode('root', null); + addNode('A', 'root'); + addNode('B', 'A'); + addNode('C', 'B'); + + const result = collectDescendantIds(nodesMap, 'A'); + + expect(result).toEqual(['B', 'C']); + }); + + it('collects multi-branch descendants', () => { + addNode('root', null); + addNode('B', 'root'); + addNode('C1', 'B'); + addNode('C2', 'B'); + addNode('D', 'C1'); + + const result = collectDescendantIds(nodesMap, 'B'); + + expect(result).toEqual(expect.arrayContaining(['C1', 'C2', 'D'])); + expect(result).toHaveLength(3); + }); + + it('returns empty array for leaf node', () => { + addNode('root', null); + addNode('A', 'root'); + + const result = collectDescendantIds(nodesMap, 'A'); + + expect(result).toEqual([]); + }); + + it('returns empty array for non-existent node', () => { + addNode('root', null); + + const result = collectDescendantIds(nodesMap, 'nonexistent'); + + expect(result).toEqual([]); + }); + + it('collects all nodes when starting from root', () => { + addNode('root', null); + addNode('A', 'root'); + addNode('B', 'root'); + addNode('C', 'A'); + + const result = collectDescendantIds(nodesMap, 'root'); + + expect(result).toEqual(expect.arrayContaining(['A', 'B', 'C'])); + expect(result).toHaveLength(3); + }); + + it('handles cycles without infinite loop', () => { + addNode('root', null); + addNode('A', 'root'); + addNode('B', 'A'); + // Create cycle: make A point to B + nodesMap.get('A')!.set('parent', 'B'); + + const result = collectDescendantIds(nodesMap, 'A'); + + expect(result).toEqual(['B']); + }); +}); diff --git a/teammapper-frontend/src/app/core/services/map-sync/map-sync.service.ts b/teammapper-frontend/src/app/core/services/map-sync/map-sync.service.ts index f666e172..52ae5f4e 100644 --- a/teammapper-frontend/src/app/core/services/map-sync/map-sync.service.ts +++ b/teammapper-frontend/src/app/core/services/map-sync/map-sync.service.ts @@ -55,6 +55,7 @@ import { findAffectedNodes, resolveMmpPropertyUpdate, sortParentFirst, + collectDescendantIds, } from './yjs-utils'; const DEFAULT_COLOR = '#000000'; @@ -1406,7 +1407,16 @@ export class MapSyncService implements OnDestroy { private writeNodeRemoveFromYDoc(nodeId: string): void { const nodesMap = this.yDoc.getMap('nodes') as Y.Map>; - nodesMap.delete(nodeId); + if (!nodesMap.has(nodeId)) return; + + const descendantIds = collectDescendantIds(nodesMap, nodeId); + + this.yDoc.transact(() => { + nodesMap.delete(nodeId); + for (const id of descendantIds) { + nodesMap.delete(id); + } + }); } private writeNodesPasteToYDoc(nodes: ExportNodeProperties[]): void { diff --git a/teammapper-frontend/src/app/core/services/map-sync/yjs-utils.ts b/teammapper-frontend/src/app/core/services/map-sync/yjs-utils.ts index ac1dca2b..0329b8e8 100644 --- a/teammapper-frontend/src/app/core/services/map-sync/yjs-utils.ts +++ b/teammapper-frontend/src/app/core/services/map-sync/yjs-utils.ts @@ -181,6 +181,44 @@ export const sortParentFirst = ( return appendOrphans(ordered, nodes); }; +// Collects all descendant node IDs by building a parent-to-children +// index in a single O(N) pass, then BFS-traversing from the given node. +export function collectDescendantIds( + nodesMap: Y.Map>, + nodeId: string +): string[] { + const childrenOf = new Map(); + nodesMap.forEach((yNode: Y.Map, key: string) => { + const parent = yNode.get('parent') as string | null; + if (parent != null) { + const siblings = childrenOf.get(parent); + if (siblings) { + siblings.push(key); + } else { + childrenOf.set(parent, [key]); + } + } + }); + + const descendants: string[] = []; + const visited = new Set([nodeId]); + const queue = [nodeId]; + let i = 0; + + while (i < queue.length) { + const children = childrenOf.get(queue[i++]) ?? []; + for (const child of children) { + if (!visited.has(child)) { + visited.add(child); + descendants.push(child); + queue.push(child); + } + } + } + + return descendants; +} + export function resolveCompoundMmpUpdates( mapping: Record, value: Record From c8a426ff322efc4c9ed626b7f65cd22f73513944 Mon Sep 17 00:00:00 2001 From: JannikStreek Date: Tue, 17 Feb 2026 20:40:07 +0100 Subject: [PATCH 2/8] add specs for yjs undo redo manager (#1163) --- .../yjs-undo-redo-manager/.openspec.yaml | 2 + .../changes/yjs-undo-redo-manager/design.md | 118 +++++++++++++++++ .../changes/yjs-undo-redo-manager/proposal.md | 36 ++++++ .../specs/yjs-bridge/spec.md | 51 ++++++++ .../specs/yjs-undo/spec.md | 120 ++++++++++++++++++ .../changes/yjs-undo-redo-manager/tasks.md | 55 ++++++++ 6 files changed, 382 insertions(+) create mode 100644 openspec/changes/yjs-undo-redo-manager/.openspec.yaml create mode 100644 openspec/changes/yjs-undo-redo-manager/design.md create mode 100644 openspec/changes/yjs-undo-redo-manager/proposal.md create mode 100644 openspec/changes/yjs-undo-redo-manager/specs/yjs-bridge/spec.md create mode 100644 openspec/changes/yjs-undo-redo-manager/specs/yjs-undo/spec.md create mode 100644 openspec/changes/yjs-undo-redo-manager/tasks.md diff --git a/openspec/changes/yjs-undo-redo-manager/.openspec.yaml b/openspec/changes/yjs-undo-redo-manager/.openspec.yaml new file mode 100644 index 00000000..c8d3976a --- /dev/null +++ b/openspec/changes/yjs-undo-redo-manager/.openspec.yaml @@ -0,0 +1,2 @@ +schema: spec-driven +created: 2026-02-17 diff --git a/openspec/changes/yjs-undo-redo-manager/design.md b/openspec/changes/yjs-undo-redo-manager/design.md new file mode 100644 index 00000000..bb822641 --- /dev/null +++ b/openspec/changes/yjs-undo-redo-manager/design.md @@ -0,0 +1,118 @@ +## Context + +TeamMapper's undo/redo is currently driven by MMP's internal `History` class — a snapshot-based system that saves a full copy of all nodes after each change and redraws the map when undoing/redoing. When the Yjs feature flag is active, the `MapSyncService` bridge writes MMP's undo/redo diffs to the Y.Doc via `writeUndoRedoDiffToYDoc()`, but this has two problems: + +1. MMP's history stack is polluted by remote changes — `addNodesFromServer` calls `addNodes()` with `updateHistory=true` (default), and `removeNode()` calls `history.save()` unconditionally. This means User A's undo stack contains User B's edits. +2. The diff-based approach writes undo results as forward operations to Y.Doc. Other clients see these as new changes, not undos. There is no per-user undo semantics. + +Yjs provides `Y.UndoManager`, which observes Y.Doc transactions tagged with a specific origin, captures deltas, and can reverse them. Each client gets its own UndoManager instance — User A's undo only reverses User A's Y.Doc writes. + +Key constraint: MMP cannot be modified. It will continue saving history snapshots internally, but when Yjs is active, nothing external reads or acts on that history. + +## Goals / Non-Goals + +**Goals:** +- Per-user collaborative undo/redo when `yjsEnabled` is true +- Toolbar buttons route to `Y.UndoManager` instead of MMP's `History` +- Reactive `canUndo`/`canRedo` state for the toolbar +- Clean lifecycle management (create/destroy UndoManager with Y.Doc) + +**Non-Goals:** +- Modifying the MMP library +- Undo/redo changes when `yjsEnabled` is false (MMP history continues as-is) +- Keyboard shortcuts (separate concern) +- Server-side changes (UndoManager is client-only) +- Grouping heuristics beyond Yjs defaults (can be tuned later) + +## Decisions + +### 1. Transaction origin tracking with a dedicated constant + +**Decision:** All local MMP-to-YDoc write operations pass `'local'` as the transaction origin. The `Y.UndoManager` is configured with `trackedOrigins: new Set(['local'])`. + +```typescript +// Every local write: +this.yDoc.transact(() => { + nodesMap.set(nodeId, yNode); +}, 'local'); + +// UndoManager setup: +new Y.UndoManager(nodesMap, { + trackedOrigins: new Set(['local']), +}); +``` + +**Affected write methods** (all in `MapSyncService`): +- `writeNodeCreateToYDoc` — needs wrapping in `transact(..., 'local')` +- `writeNodeUpdateToYDoc` — single `yNode.set()` call, needs wrapping +- `writeNodeRemoveFromYDoc` — already uses `transact()`, add origin +- `writeNodesPasteToYDoc` — already uses `transact()`, add origin +- `writeMapOptionsToYDoc` — individual `set()` calls, needs wrapping + +**Import is excluded:** `writeImportToYDoc` uses a separate origin (`'import'`) so full map replacements are not undoable. Undoing an import would be destructive and confusing. + +**Alternative considered:** Using the Y.Doc `clientID` as the origin. Rejected because `clientID` changes on reconnect (new Y.Doc instance), and a string constant is simpler and more explicit. + +### 2. UndoManager scope: `nodesMap` only (initially) + +**Decision:** Track only `Y.Map("nodes")`. Map options (`fontMaxSize`, `fontMinSize`, `fontIncrement`) are excluded from undo tracking. + +**Rationale:** Map options are global settings rarely changed. Including them would mean undoing a node edit could unexpectedly revert a font size change made between edits. Keeping the scope to nodes matches user expectations — undo reverses the last node-level operation. + +**Alternative considered:** Tracking both `[nodesMap, optionsMap]`. Deferred — can be added if users request it. + +### 3. Echo prevention: origin-based filtering in `observeDeep` + +**Decision:** Replace the current `transaction.local` check with origin-based filtering: + +``` +Current: if (transaction.local) return; +Proposed: if (transaction.local && transaction.origin !== this.yUndoManager) return; +``` + +**Why this is needed:** When `Y.UndoManager.undo()` executes, it creates a local transaction (origin = the UndoManager instance). The current observer skips all local transactions, which means undo changes would never reach MMP. The updated check allows UndoManager-originated transactions through so the bridge can apply node adds/updates/deletes to MMP for rendering. + +The same logic applies to the `mapOptions` observer, though it is less critical since map options are excluded from undo tracking. + +### 4. Toolbar routing: conditional delegation via `MapSyncService` + +**Decision:** `MapSyncService` exposes `undo()`, `redo()`, `canUndo$`, and `canRedo$`. The toolbar checks `yjsEnabled` (via `SettingsService`) to decide whether to call `mapSyncService.undo()` or `mmpService.undo()`. + +**`canUndo$` / `canRedo$` implementation:** `Y.UndoManager` emits `'stack-item-added'` and `'stack-item-popped'` events. On each event, emit the current `undoStack.length > 0` / `redoStack.length > 0` to a `BehaviorSubject`. + +**Alternative considered:** Having `MapSyncService.undo()` internally check `yjsEnabled` and delegate to either Y.UndoManager or MMP. Rejected because the toolbar already needs to know `yjsEnabled` for the `canUndoRedo` check — it's cleaner to have the routing in one place (the toolbar) than hidden inside the service. + +### 5. Lifecycle: create with Y.Doc, destroy on reset + +**Decision:** `Y.UndoManager` is created in `initYjs()` after the Y.Doc and nodesMap are ready (after first sync). Destroyed in `resetYjs()` before `yDoc.destroy()`. + +``` +initYjs() → Y.Doc created → WebSocket connected → first sync + → handleFirstYjsSync() → loadMapFromYDoc() → setupYjsNodesObserver() + → CREATE Y.UndoManager (after observer, so initial load is not captured) + +resetYjs() → DESTROY Y.UndoManager → unsubscribe listeners → destroy provider → destroy Y.Doc +``` + +**Critical timing:** The UndoManager must be created AFTER `loadMapFromYDoc()` completes. If created before, the initial map hydration (which writes nodes to Y.Doc during first sync) would fill the undo stack with "undo the entire map" operations. + +### 6. Removal of diff-based undo bridge + +**Decision:** Remove `setupYjsUndoRedoHandlers()` and `writeUndoRedoDiffToYDoc()` (plus its helpers `writeAddedNodesToYDoc`, `writeUpdatedNodesToYDoc`, `writeDeletedNodesFromYDoc`). These become dead code — Y.UndoManager handles undo/redo natively through Y.Doc without needing to compute and apply diffs. + +The MMP `undo`/`redo` events are no longer subscribed to in the Yjs path. MMP's `History.undo()` / `History.redo()` are never called when Yjs is active. + +## Risks / Trade-offs + +**[MMP history accumulates unused snapshots]** → MMP internally calls `history.save()` on node operations, building up a snapshot array that nobody reads. Mitigation: Mind maps are small (tens to hundreds of nodes). Memory overhead is negligible. Fixing this would require MMP modifications, which is out of scope. + +**[UndoManager capture granularity]** → `Y.UndoManager` uses a default `captureTimeout` of 500ms — changes within this window are grouped into a single undo step. When dragging a node, rapid coordinate updates may group into one undo step (desirable). But two quick edits to different nodes within 500ms would also group (potentially surprising). Mitigation: 500ms is the Yjs default and works well for most editors. Can be tuned later if user feedback warrants it. + +**[Undo after reconnect]** → If a client disconnects and reconnects, a new `Y.Doc` and `Y.UndoManager` are created. The undo stack is lost. Mitigation: This matches standard editor behavior (e.g., refreshing a Google Doc clears undo history). The Y.Doc state itself is preserved — only the local undo stack resets. + +**[Observer ordering with UndoManager]** → The `observeDeep` callback needs to handle UndoManager-originated transactions (adds, deletes, updates) the same way it handles remote changes. The existing `applyRemoteNodeAdd`/`applyRemoteNodeDelete`/`applyYDocPropertyToMmp` methods should work unchanged since they only care about Y.Doc state, not transaction origin. Mitigation: Thorough testing of undo/redo for create, update, delete, and paste operations. + +## Open Questions + +- **Should `captureTimeout` be configurable?** The default 500ms groups rapid changes. For node dragging this is good, but it could group unrelated edits. Worth tuning based on user testing? +- **Undo scope for multi-node operations:** When a user deletes a node with descendants, the bridge deletes all descendants in one transaction. UndoManager captures this as one undo step (restores parent + all children). Is this the desired behavior, or should each node be a separate undo step? diff --git a/openspec/changes/yjs-undo-redo-manager/proposal.md b/openspec/changes/yjs-undo-redo-manager/proposal.md new file mode 100644 index 00000000..3d816b0c --- /dev/null +++ b/openspec/changes/yjs-undo-redo-manager/proposal.md @@ -0,0 +1,36 @@ +## Why + +When Yjs is active, undo/redo uses MMP's snapshot-based history which has no concept of "my changes vs your changes." MMP's history stack gets polluted by remote changes (both `addNodesFromServer` and `removeNode` unconditionally call `history.save()`), so pressing undo may reverse another user's edit. Yjs provides `Y.UndoManager` which tracks changes by transaction origin, giving proper per-user collaborative undo — User A's undo only reverses User A's changes, regardless of concurrent edits by others. + +## What Changes + +- **Add `Y.UndoManager` to `MapSyncService`**: When `yjsEnabled`, create a `Y.UndoManager` tracking the `nodesMap` (and optionally `mapOptions`). All local write operations use a tracked transaction origin (`'local'`). Import operations use a separate untracked origin so they are not undoable. +- **Route toolbar undo/redo through `MapSyncService`**: When `yjsEnabled`, toolbar buttons and keyboard shortcuts call `mapSyncService.undo()`/`redo()` which delegate to `Y.UndoManager`, bypassing MMP's `History` entirely. When Yjs is disabled, the existing MMP undo/redo path is unchanged. +- **Expose `canUndo`/`canRedo` observables**: `MapSyncService` exposes reactive state derived from `Y.UndoManager.undoStack`/`redoStack` so the toolbar can enable/disable buttons. +- **Update `observeDeep` echo prevention**: Allow `Y.UndoManager`-originated transactions to pass through to MMP (they are local but need to update the renderer). +- **Remove diff-based undo bridge**: `setupYjsUndoRedoHandlers()` and `writeUndoRedoDiffToYDoc()` are no longer needed when `Y.UndoManager` handles undo natively through Y.Doc. +- **MMP history becomes inert when Yjs is active**: MMP internally still calls `history.save()` (we don't modify MMP), but no external code reads or acts on it. This is acceptable — mind maps are small and the memory overhead is negligible. + +## Non-goals + +- Modifying the MMP library in any way +- Disabling MMP's internal `history.save()` calls (would require MMP changes) +- Adding undo/redo support when Yjs is disabled (existing MMP history continues to work) +- Keyboard shortcuts for undo/redo (can be added separately; this change focuses on the undo mechanism) +- Server-side changes (Y.UndoManager is purely a frontend concern) + +## Capabilities + +### New Capabilities +- `yjs-undo`: Y.UndoManager lifecycle, transaction origin tracking, toolbar integration, canUndo/canRedo observables, and echo prevention for undo-originated transactions + +### Modified Capabilities +- `yjs-bridge`: The MMP-to-YDoc write operations need transaction origins on all `yDoc.transact()` calls, and the YDoc-to-MMP observer needs updated echo prevention logic to allow UndoManager-originated transactions through + +## Impact + +- **`MapSyncService`** (`map-sync.service.ts`): New `Y.UndoManager` field, `undo()`/`redo()` public methods, `canUndo$`/`canRedo$` observables, transaction origins on all write methods, updated observer logic, removal of `setupYjsUndoRedoHandlers` and `writeUndoRedoDiffToYDoc` +- **Toolbar** (`toolbar.component.ts/html`): Conditional routing of undo/redo clicks and `canUndoRedo` check based on `yjsEnabled` +- **No new dependencies**: `Y.UndoManager` is part of the `yjs` package already installed +- **No backend changes**: UndoManager is client-side only +- **No breaking changes**: Behavior is unchanged when `yjsEnabled` is false diff --git a/openspec/changes/yjs-undo-redo-manager/specs/yjs-bridge/spec.md b/openspec/changes/yjs-undo-redo-manager/specs/yjs-bridge/spec.md new file mode 100644 index 00000000..f696b5b6 --- /dev/null +++ b/openspec/changes/yjs-undo-redo-manager/specs/yjs-bridge/spec.md @@ -0,0 +1,51 @@ +## MODIFIED Requirements + +### Requirement: Local MMP events write to Y.Doc +When the user performs an action in MMP (create, update, remove node), the MapSyncService bridge SHALL write the change to the local Y.Doc. The bridge SHALL NOT send individual network messages — Yjs handles synchronization automatically. All local write operations SHALL use `'local'` as the transaction origin so that `Y.UndoManager` can track them. + +#### Scenario: User creates a node +- **WHEN** MMP fires a `nodeCreate` event with the new node's `ExportNodeProperties` +- **THEN** the bridge SHALL create a new Y.Map entry in `yDoc.getMap('nodes')` with the node's ID as key and all properties as values +- **AND** the write SHALL be wrapped in `yDoc.transact(() => { ... }, 'local')` + +#### Scenario: User updates a node property +- **WHEN** MMP fires a `nodeUpdate` event with the updated property and value +- **THEN** the bridge SHALL update the corresponding property on the node's Y.Map entry in the `nodes` map +- **AND** the write SHALL be wrapped in `yDoc.transact(() => { ... }, 'local')` + +#### Scenario: User removes a node +- **WHEN** MMP fires a `nodeRemove` event with the removed node's properties +- **THEN** the bridge SHALL delete the node's entry and all descendant entries from `yDoc.getMap('nodes')` +- **AND** the `yDoc.transact()` call SHALL use `'local'` as the origin + +#### Scenario: User pastes multiple nodes +- **WHEN** MMP fires a `nodePaste` event with an array of node properties +- **THEN** the bridge SHALL add all nodes to the Y.Doc `nodes` map within a single `yDoc.transact()` call +- **AND** the `yDoc.transact()` call SHALL use `'local'` as the origin + +#### Scenario: User updates map options +- **WHEN** the user changes map options (e.g., map name) +- **THEN** the bridge SHALL update the corresponding fields in `yDoc.getMap('mapOptions')` +- **AND** the write SHALL be wrapped in `yDoc.transact(() => { ... }, 'local')` + +### Requirement: Echo prevention +The bridge SHALL prevent echo loops where a local MMP event writes to Y.Doc, which then triggers a Y.Doc observation that would re-apply the same change to MMP. The bridge SHALL use origin-based filtering: skip transactions that are local AND whose origin is NOT the `Y.UndoManager` instance. UndoManager-originated transactions are local but SHALL be applied to MMP because MMP was not the source of the change. + +#### Scenario: Local change does not echo back +- **WHEN** the user creates a node locally (MMP event -> Y.Doc write with origin `'local'`) +- **THEN** the Y.Doc observer SHALL detect the transaction as local with origin `'local'` and SHALL NOT apply it back to MMP + +#### Scenario: Remote change is applied +- **WHEN** a remote client creates a node (Y.Doc sync update received) +- **THEN** the Y.Doc observer SHALL detect the change as `transaction.local === false` and SHALL apply it to MMP + +#### Scenario: UndoManager change is applied to MMP +- **WHEN** the user triggers undo/redo and `Y.UndoManager` creates a local transaction (origin = UndoManager instance) +- **THEN** the Y.Doc observer SHALL detect `transaction.origin === yUndoManager` and SHALL apply the changes to MMP +- **AND** MMP SHALL re-render the affected nodes + +## REMOVED Requirements + +### Requirement: Local undo/redo stays local +**Reason**: Replaced by `Y.UndoManager` in the `yjs-undo` capability. MMP's snapshot-based undo/redo is no longer used when Yjs is active. The diff-based bridge (`writeUndoRedoDiffToYDoc`) that forwarded MMP undo/redo results to Y.Doc is removed. +**Migration**: Undo/redo is now handled by `Y.UndoManager` which operates directly on Y.Doc. The toolbar routes to `MapSyncService.undo()`/`redo()` instead of `MmpService.undo()`/`redo()` when `yjsEnabled` is true. diff --git a/openspec/changes/yjs-undo-redo-manager/specs/yjs-undo/spec.md b/openspec/changes/yjs-undo-redo-manager/specs/yjs-undo/spec.md new file mode 100644 index 00000000..4b351018 --- /dev/null +++ b/openspec/changes/yjs-undo-redo-manager/specs/yjs-undo/spec.md @@ -0,0 +1,120 @@ +## ADDED Requirements + +### Requirement: Y.UndoManager lifecycle +When `yjsEnabled` is true, the `MapSyncService` SHALL create a `Y.UndoManager` instance tracking the `nodesMap` (`yDoc.getMap('nodes')`). The UndoManager SHALL be configured with `trackedOrigins: new Set(['local'])`. The UndoManager SHALL be created AFTER the first Y.Doc sync and map load completes, so that the initial hydration is not captured as undoable operations. The UndoManager SHALL be destroyed in `resetYjs()` before the Y.Doc is destroyed. + +#### Scenario: UndoManager created after first sync +- **WHEN** the Y.Doc completes its first sync and `loadMapFromYDoc()` finishes +- **THEN** the service SHALL create a `Y.UndoManager` on the `nodesMap` with `trackedOrigins: new Set(['local'])` +- **AND** the UndoManager's undo stack SHALL be empty (initial load not captured) + +#### Scenario: UndoManager destroyed on reset +- **WHEN** `resetYjs()` is called (e.g., navigating away from a map or disconnecting) +- **THEN** the service SHALL call `yUndoManager.destroy()` before destroying the Y.Doc + +#### Scenario: UndoManager not created when Yjs disabled +- **WHEN** `yjsEnabled` is false +- **THEN** the service SHALL NOT create a `Y.UndoManager` instance + +### Requirement: Transaction origin on local writes +All local MMP-to-YDoc write operations SHALL use `'local'` as the transaction origin so that `Y.UndoManager` captures them. Import operations SHALL use a separate origin (`'import'`) that is NOT tracked by the UndoManager. + +#### Scenario: Node create uses tracked origin +- **WHEN** the user creates a node and the bridge writes to Y.Doc +- **THEN** the write SHALL be wrapped in `yDoc.transact(() => { ... }, 'local')` +- **AND** the UndoManager SHALL capture the operation in its undo stack + +#### Scenario: Node update uses tracked origin +- **WHEN** the user updates a node property and the bridge writes to Y.Doc +- **THEN** the write SHALL be wrapped in `yDoc.transact(() => { ... }, 'local')` + +#### Scenario: Node remove uses tracked origin +- **WHEN** the user removes a node and the bridge deletes from Y.Doc +- **THEN** the `yDoc.transact()` call SHALL include `'local'` as the origin + +#### Scenario: Node paste uses tracked origin +- **WHEN** the user pastes nodes and the bridge writes to Y.Doc +- **THEN** the `yDoc.transact()` call SHALL include `'local'` as the origin + +#### Scenario: Map options update uses tracked origin +- **WHEN** the user changes map options and the bridge writes to Y.Doc +- **THEN** the write SHALL be wrapped in `yDoc.transact(() => { ... }, 'local')` + +#### Scenario: Map import uses untracked origin +- **WHEN** the user imports a map and the bridge writes to Y.Doc +- **THEN** the `yDoc.transact()` call SHALL use `'import'` as the origin +- **AND** the UndoManager SHALL NOT capture the import operation + +### Requirement: Undo and redo via Y.UndoManager +When `yjsEnabled` is true, the `MapSyncService` SHALL expose public `undo()` and `redo()` methods that delegate to `Y.UndoManager.undo()` and `Y.UndoManager.redo()` respectively. MMP's `History.undo()` and `History.redo()` SHALL NOT be called when Yjs is active. + +#### Scenario: User triggers undo with Yjs active +- **WHEN** the user presses the undo button and `yjsEnabled` is true +- **THEN** `mapSyncService.undo()` SHALL be called +- **AND** `Y.UndoManager.undo()` SHALL reverse the user's last tracked Y.Doc change +- **AND** `mmpService.undo()` SHALL NOT be called + +#### Scenario: User triggers redo with Yjs active +- **WHEN** the user presses the redo button and `yjsEnabled` is true +- **THEN** `mapSyncService.redo()` SHALL be called +- **AND** `Y.UndoManager.redo()` SHALL reapply the user's last undone change +- **AND** `mmpService.redo()` SHALL NOT be called + +#### Scenario: Per-user undo isolation +- **WHEN** User A and User B both edit nodes, and User A presses undo +- **THEN** only User A's last change SHALL be reversed +- **AND** User B's changes SHALL remain intact + +#### Scenario: Undo when stack is empty +- **WHEN** the user presses undo but the UndoManager's undo stack is empty +- **THEN** the `undo()` call SHALL be a no-op (Y.UndoManager handles this gracefully) + +### Requirement: Reactive canUndo and canRedo state +The `MapSyncService` SHALL expose `canUndo$` and `canRedo$` as `Observable` so the toolbar can reactively enable/disable undo/redo buttons when Yjs is active. + +#### Scenario: canUndo updates after user edit +- **WHEN** the user makes an edit that is captured by the UndoManager +- **THEN** `canUndo$` SHALL emit `true` + +#### Scenario: canUndo updates after undo empties stack +- **WHEN** the user undoes all operations and the undo stack becomes empty +- **THEN** `canUndo$` SHALL emit `false` + +#### Scenario: canRedo updates after undo +- **WHEN** the user performs an undo +- **THEN** `canRedo$` SHALL emit `true` + +#### Scenario: canRedo resets after new edit +- **WHEN** the user makes a new edit after undoing (clearing the redo stack) +- **THEN** `canRedo$` SHALL emit `false` + +### Requirement: Toolbar conditional routing +The toolbar SHALL route undo/redo actions based on `yjsEnabled`. When `yjsEnabled` is true, the toolbar SHALL call `mapSyncService.undo()`/`redo()` and use `canUndo$`/`canRedo$` for button state. When `yjsEnabled` is false, the toolbar SHALL continue calling `mmpService.undo()`/`redo()` and checking `mmpService.history()` for button state. + +#### Scenario: Toolbar undo with Yjs active +- **WHEN** the user clicks the undo button and `yjsEnabled` is true +- **THEN** the toolbar SHALL call `mapSyncService.undo()` + +#### Scenario: Toolbar undo with Yjs inactive +- **WHEN** the user clicks the undo button and `yjsEnabled` is false +- **THEN** the toolbar SHALL call `mmpService.undo()` + +#### Scenario: Toolbar button state with Yjs active +- **WHEN** `yjsEnabled` is true +- **THEN** the undo button's disabled state SHALL be derived from `canUndo$` (disabled when false) +- **AND** the redo button's disabled state SHALL be derived from `canRedo$` (disabled when false) + +#### Scenario: Toolbar button state with Yjs inactive +- **WHEN** `yjsEnabled` is false +- **THEN** the undo/redo button disabled state SHALL continue using `mmpService.history().snapshots.length > 1` + +### Requirement: Removal of diff-based undo bridge +When `yjsEnabled` is true, the `MapSyncService` SHALL NOT subscribe to MMP's `undo` and `redo` events. The `setupYjsUndoRedoHandlers()` method and `writeUndoRedoDiffToYDoc()` method (and its helpers) SHALL be removed. + +#### Scenario: MMP undo event not subscribed in Yjs path +- **WHEN** `yjsEnabled` is true and the Yjs bridge is initialized +- **THEN** the service SHALL NOT subscribe to `mmpService.on('undo')` or `mmpService.on('redo')` + +#### Scenario: Diff-based write methods removed +- **WHEN** the codebase is updated +- **THEN** `writeUndoRedoDiffToYDoc`, `writeAddedNodesToYDoc`, `writeUpdatedNodesToYDoc`, and `writeDeletedNodesFromYDoc` SHALL no longer exist in `MapSyncService` diff --git a/openspec/changes/yjs-undo-redo-manager/tasks.md b/openspec/changes/yjs-undo-redo-manager/tasks.md new file mode 100644 index 00000000..e0b73450 --- /dev/null +++ b/openspec/changes/yjs-undo-redo-manager/tasks.md @@ -0,0 +1,55 @@ +## 1. Add transaction origins to all Y.Doc write operations + +- [ ] 1.1 Wrap `writeNodeCreateToYDoc` in `yDoc.transact(() => { ... }, 'local')` — currently writes without a transaction +- [ ] 1.2 Wrap `writeNodeUpdateToYDoc` in `yDoc.transact(() => { ... }, 'local')` — currently a bare `yNode.set()` call +- [ ] 1.3 Add `'local'` origin to `writeNodeRemoveFromYDoc` — already uses `yDoc.transact()`, add second argument +- [ ] 1.4 Add `'local'` origin to `writeNodesPasteToYDoc` — already uses `yDoc.transact()`, add second argument +- [ ] 1.5 Wrap `writeMapOptionsToYDoc` in `yDoc.transact(() => { ... }, 'local')` — currently individual `set()` calls +- [ ] 1.6 Change `writeImportToYDoc` to use `'import'` origin — already uses `yDoc.transact()`, change to untracked origin +- [ ] 1.7 Write unit tests verifying each write method uses the correct transaction origin + +## 2. Add Y.UndoManager lifecycle to MapSyncService + +- [ ] 2.1 Add `private yUndoManager: Y.UndoManager | null = null` field to `MapSyncService` +- [ ] 2.2 Create `initYjsUndoManager()` method that instantiates `Y.UndoManager` on `nodesMap` with `trackedOrigins: new Set(['local'])` +- [ ] 2.3 Call `initYjsUndoManager()` in `handleFirstYjsSync()` AFTER `loadMapFromYDoc()` and `setupYjsNodesObserver()` complete +- [ ] 2.4 Add `yUndoManager.destroy()` to `resetYjs()` before `yDoc.destroy()` +- [ ] 2.5 Write unit tests: UndoManager created after first sync, destroyed on reset, undo stack empty after init + +## 3. Update echo prevention in Y.Doc observers + +- [ ] 3.1 Change `setupYjsNodesObserver` filter from `if (transaction.local) return` to `if (transaction.local && transaction.origin !== this.yUndoManager) return` +- [ ] 3.2 Apply same origin-based filter in `setupYjsMapOptionsObserver` if applicable +- [ ] 3.3 Write unit tests: local edit skipped, remote edit applied, UndoManager-originated edit applied to MMP + +## 4. Expose undo/redo methods and reactive state + +- [ ] 4.1 Add public `undo()` method that calls `this.yUndoManager?.undo()` +- [ ] 4.2 Add public `redo()` method that calls `this.yUndoManager?.redo()` +- [ ] 4.3 Add `canUndo$` and `canRedo$` as `BehaviorSubject` fields, exposed as `Observable` +- [ ] 4.4 Subscribe to UndoManager `stack-item-added` and `stack-item-popped` events to update `canUndo$`/`canRedo$` +- [ ] 4.5 Clean up UndoManager event listeners in `resetYjs()` +- [ ] 4.6 Write unit tests: canUndo$/canRedo$ emit correctly after edit, undo, redo, and stack empty + +## 5. Remove diff-based undo bridge + +- [ ] 5.1 Remove `setupYjsUndoRedoHandlers()` method and its call site in `setupYjsMmpEventHandlers()` +- [ ] 5.2 Remove `writeUndoRedoDiffToYDoc()`, `writeAddedNodesToYDoc()`, `writeUpdatedNodesToYDoc()`, `writeDeletedNodesFromYDoc()` methods +- [ ] 5.3 Remove unused imports (`MapDiff`, `SnapshotChanges` from `@mmp/map/handlers/history`) if no longer referenced +- [ ] 5.4 Update existing unit tests that reference the removed methods + +## 6. Update toolbar to route undo/redo conditionally + +- [ ] 6.1 Inject `MapSyncService` and `SettingsService` into toolbar component (check if already available) +- [ ] 6.2 Add `yjsEnabled` property to toolbar, read from `SettingsService.getCachedSystemSettings().featureFlags.yjs` +- [ ] 6.3 Update undo button `(click)` to call `mapSyncService.undo()` when `yjsEnabled`, else `mmpService.undo()` +- [ ] 6.4 Update redo button `(click)` to call `mapSyncService.redo()` when `yjsEnabled`, else `mmpService.redo()` +- [ ] 6.5 Update `canUndoRedo` getter: when `yjsEnabled`, subscribe to `canUndo$`/`canRedo$`; when not, use existing `mmpService.history()` check +- [ ] 6.6 Write unit tests for toolbar conditional routing (both Yjs and non-Yjs paths) + +## 7. Integration testing + +- [ ] 7.1 Write integration test: create node, undo reverses it, redo restores it +- [ ] 7.2 Write integration test: undo after import is no-op (import not captured) +- [ ] 7.3 Write integration test: undo/redo button disabled state matches UndoManager stack +- [ ] 7.4 Verify existing Playwright e2e tests pass (undo/redo toolbar buttons still functional) From fc98d3e6fd3bfd9972796a9f94b0d519f1e8e544 Mon Sep 17 00:00:00 2001 From: JannikStreek Date: Wed, 18 Feb 2026 19:07:32 +0100 Subject: [PATCH 3/8] spec: yjs undo redo manager (#1165) --- openspec/changes/yjs-introduction/design.md | 10 + .../yjs-introduction/specs/yjs-sync/spec.md | 16 + openspec/changes/yjs-introduction/tasks.md | 1 + .../changes/yjs-undo-redo-manager/design.md | 38 +- .../changes/yjs-undo-redo-manager/proposal.md | 4 +- .../specs/yjs-bridge/spec.md | 37 ++ .../specs/yjs-undo/spec.md | 23 + .../changes/yjs-undo-redo-manager/tasks.md | 88 ++-- .../map/controllers/maps.controller.spec.ts | 12 +- .../src/map/controllers/maps.controller.ts | 13 +- .../controllers/yjs-gateway.service.spec.ts | 453 +++++------------- .../map/controllers/yjs-gateway.service.ts | 29 +- teammapper-backend/src/map/map.module.ts | 13 +- .../src/map/utils/yjsProtocol.ts | 10 + teammapper-backend/test/jest-setup.ts | 10 +- .../mmp/src/map/handlers/nodes.ts | 10 +- .../map-sync/map-sync.service.spec.ts | 117 +++++ .../services/map-sync/map-sync.service.ts | 178 ++++--- .../map-sync/yjs-undo-manager.spec.ts | 379 +++++++++++++++ .../src/app/core/services/mmp/mmp.service.ts | 6 +- .../components/toolbar/toolbar.component.html | 8 +- .../toolbar/toolbar.component.spec.ts | 403 ++++++++++------ .../components/toolbar/toolbar.component.ts | 56 ++- 23 files changed, 1233 insertions(+), 681 deletions(-) create mode 100644 teammapper-frontend/src/app/core/services/map-sync/yjs-undo-manager.spec.ts diff --git a/openspec/changes/yjs-introduction/design.md b/openspec/changes/yjs-introduction/design.md index 133046ed..cc913a34 100644 --- a/openspec/changes/yjs-introduction/design.md +++ b/openspec/changes/yjs-introduction/design.md @@ -132,6 +132,16 @@ Y.Doc (one per map) - When applying a remote Y.Doc change to MMP, call MMP methods with `notifyWithEvent: false` to suppress the MMP event - When writing a local MMP event to Y.Doc, the Y.Doc `observe` callback checks `transaction.local` to distinguish local vs remote changes +### 5a. Sync progress feedback — loading toast during initial sync + +**Decision:** Show a non-auto-dismissing info toast ("Syncing map...") from the moment the `WebsocketProvider` is created until the first Y.Doc sync completes (`handleFirstYjsSync`). Dismiss it in `resetYjs()` as well for cleanup on disconnect. + +**Rationale:** After the race condition fix that defers edit mode until Yjs sync completes, the user sees the map rendered (from the REST API) in a read-only state with no indication that anything is loading. The toast bridges this gap. Using `toastrService.info()` with `timeOut: 0` follows the same pattern as the existing `MAP_IMPORT_IN_PROGRESS` toast. The toast ID is stored in `yjsSyncToastId` and removed via `toastrService.remove()` — simpler than a custom loading indicator and consistent with the existing UX. + +**Alternatives considered:** +- *Spinner overlay on the map*: More visually prominent but requires new UI component work. The toast is lightweight and already part of the design language. +- *Disable map rendering until sync*: Would delay the initial visual, making the app feel slower. Showing the REST-loaded map with a toast is a better experience. + ### 6. Presence: Yjs Awareness API **Decision:** Use the Yjs Awareness protocol (bundled with y-websocket) for client presence, colors, and node selection. Each client sets its awareness state with `{ color, selectedNodeId, userName }`. diff --git a/openspec/changes/yjs-introduction/specs/yjs-sync/spec.md b/openspec/changes/yjs-introduction/specs/yjs-sync/spec.md index 32e884a6..6b43a271 100644 --- a/openspec/changes/yjs-introduction/specs/yjs-sync/spec.md +++ b/openspec/changes/yjs-introduction/specs/yjs-sync/spec.md @@ -71,6 +71,22 @@ The frontend SHALL connect to the Yjs WebSocket endpoint using `y-websocket`'s ` - **WHEN** the `WebsocketProvider` reconnects after a disconnection - **THEN** the Yjs sync protocol SHALL automatically reconcile the local Y.Doc with the server Y.Doc without a full map reload +### Requirement: Connection status observable +The frontend SHALL expose a reactive `ConnectionStatus` observable (`'connected' | 'disconnected' | null`) via `MapSyncService`. UI components SHALL subscribe to this observable to present connection state (e.g., showing a disconnect dialog). The service SHALL NOT directly control UI elements like toasts or dialogs. + +#### Scenario: Initial sync completes +- **WHEN** the `WebsocketProvider` fires its first `sync` event with `synced: true` +- **THEN** the connection status SHALL transition to `'connected'` +- **AND** edit mode and Y.Doc observers SHALL be initialized + +#### Scenario: WebSocket disconnects +- **WHEN** the WebSocket connection is lost +- **THEN** the connection status SHALL transition to `'disconnected'` + +#### Scenario: Connection reset during cleanup +- **WHEN** the Yjs connection is reset (e.g., navigating away or switching maps) +- **THEN** the connection status SHALL be reset to `null` as part of `resetYjs()` cleanup + ### Requirement: Socket.io removal The server SHALL NOT use Socket.io for any data synchronization or presence operations. The `@nestjs/platform-socket.io`, `socket.io`, and `socket.io-client` dependencies SHALL be removed. The frontend SHALL NOT import or use `socket.io-client`. diff --git a/openspec/changes/yjs-introduction/tasks.md b/openspec/changes/yjs-introduction/tasks.md index 4108839b..1bb1e04b 100644 --- a/openspec/changes/yjs-introduction/tasks.md +++ b/openspec/changes/yjs-introduction/tasks.md @@ -84,6 +84,7 @@ - [x] 4.15 Implement Yjs Awareness — node highlighting: observe remote clients' `selectedNodeId` changes, call `mmpService.highlightNode()` with their color, ignore references to non-existent nodes - [x] 4.16 Implement cleanup on destroy: disconnect `WebsocketProvider`, destroy Y.Doc, remove observers - [x] 4.17 Verify frontend builds, lints, and unit tests pass with flag off (Socket.io path unchanged) +- [x] 4.18 Show sync loading toast: display a non-auto-dismissing info toast (`TOASTS.WARNINGS.YJS_SYNC_IN_PROGRESS`) when the `WebsocketProvider` is created, dismiss it in `handleFirstYjsSync` and `resetYjs`. Add `yjsSyncToastId` property, `showSyncLoadingToast()`, and `dismissSyncLoadingToast()` methods. Add i18n keys to all 8 locale files. Add unit tests for toast lifecycle. ## 5. Integration Testing & Flag Activation diff --git a/openspec/changes/yjs-undo-redo-manager/design.md b/openspec/changes/yjs-undo-redo-manager/design.md index bb822641..eca3b2c2 100644 --- a/openspec/changes/yjs-undo-redo-manager/design.md +++ b/openspec/changes/yjs-undo-redo-manager/design.md @@ -102,6 +102,39 @@ resetYjs() → DESTROY Y.UndoManager → unsubscribe listeners → destroy provi The MMP `undo`/`redo` events are no longer subscribed to in the Yjs path. MMP's `History.undo()` / `History.redo()` are never called when Yjs is active. +### 7. Coordinate preservation on node restore: conditional calculation in `addNode()` + +**Problem:** When `Y.UndoManager.undo()` restores deleted nodes, the Yjs observer calls `applyRemoteNodeAdd()` → `addNodesFromServer()` → `addNode()`. At `nodes.ts:124`, `addNode()` unconditionally calls `calculateCoordinates(node)`, overwriting the correct coordinates from Y.Doc with freshly calculated "append to bottom of siblings" positions. This also affects remote node additions from other clients. + +**Decision:** Make `calculateCoordinates()` conditional — only recalculate when the incoming properties don't already contain valid coordinates. This is a minimal, targeted change to the MMP library (`nodes.ts:124`). + +```typescript +// Before (bug): +node.coordinates = this.calculateCoordinates(node); + +// After (fix): +if (!properties.coordinates?.x && !properties.coordinates?.y && !node.isRoot) { + node.coordinates = this.calculateCoordinates(node); +} +``` + +**Alternative considered:** Reuse the legacy `history.ts:redraw()` method — on undo/redo, extract a full snapshot from Y.Doc via `extractSnapshotFromYDoc()`, then call `redraw()` to tear down and rebuild the entire MMP node tree. Rejected because: +- Rebuilds every node even for single-property undos (rename, color change) +- `redraw()` is private and would need exposure +- Requires suppressing the Yjs observer during rebuild to avoid event cascading +- Only fixes undo/redo — remote node additions still lose coordinates through `addNode()` +- The conditional approach is more surgical and fixes both undo/redo and remote adds + +**Precedent:** `applyCoordinatesToMapSnapshot()` (`nodes.ts:830-862`) already uses this pattern — it only calculates coordinates when `!node.coordinates`. + +**Undo stack integrity:** The `addNode()` path called from the observer uses `notifyWithEvent=false`, so `Event.nodeCreate` is not fired and `writeNodeCreateToYDoc()` is never called. No `doc.transact(..., 'local')` occurs, so the UndoManager does not capture individual node restores as new tracked changes. Branch delete/restore remains one atomic undo stack item through any number of undo/redo cycles. + +### 8. Parent-first ordering on batch node restore + +**Problem:** `keysChanged.forEach` in `handleTopLevelNodeChanges` (`map-sync.service.ts:1557`) iterates in Y.Map internal order, which may not be parent-first. A child node processed before its parent causes `getNode(parentId)` to return `undefined`. + +**Decision:** When the observer detects multiple node additions in a single transaction, collect all added keys first, then sort parent-first using the existing `sortParentFirst()` utility from `yjs-utils.ts` before applying to MMP. + ## Risks / Trade-offs **[MMP history accumulates unused snapshots]** → MMP internally calls `history.save()` on node operations, building up a snapshot array that nobody reads. Mitigation: Mind maps are small (tens to hundreds of nodes). Memory overhead is negligible. Fixing this would require MMP modifications, which is out of scope. @@ -115,4 +148,7 @@ The MMP `undo`/`redo` events are no longer subscribed to in the Yjs path. MMP's ## Open Questions - **Should `captureTimeout` be configurable?** The default 500ms groups rapid changes. For node dragging this is good, but it could group unrelated edits. Worth tuning based on user testing? -- **Undo scope for multi-node operations:** When a user deletes a node with descendants, the bridge deletes all descendants in one transaction. UndoManager captures this as one undo step (restores parent + all children). Is this the desired behavior, or should each node be a separate undo step? + +## Resolved Questions + +- **Undo scope for multi-node operations:** When a user deletes a node with descendants, the bridge deletes all descendants in one transaction. UndoManager captures this as one undo step (restores parent + all children). This IS the desired behavior — the branch should always be treated atomically. The `addNode()` path called from the observer does not write back to Y.Doc (no `'local'` origin transactions), so the undo stack is never corrupted with individual node entries. diff --git a/openspec/changes/yjs-undo-redo-manager/proposal.md b/openspec/changes/yjs-undo-redo-manager/proposal.md index 3d816b0c..463f0924 100644 --- a/openspec/changes/yjs-undo-redo-manager/proposal.md +++ b/openspec/changes/yjs-undo-redo-manager/proposal.md @@ -13,8 +13,8 @@ When Yjs is active, undo/redo uses MMP's snapshot-based history which has no con ## Non-goals -- Modifying the MMP library in any way -- Disabling MMP's internal `history.save()` calls (would require MMP changes) +- Making more than minimal, targeted changes to the MMP library (only bug fixes required for correct undo/redo behavior) +- Disabling MMP's internal `history.save()` calls (would require broader MMP refactoring) - Adding undo/redo support when Yjs is disabled (existing MMP history continues to work) - Keyboard shortcuts for undo/redo (can be added separately; this change focuses on the undo mechanism) - Server-side changes (Y.UndoManager is purely a frontend concern) diff --git a/openspec/changes/yjs-undo-redo-manager/specs/yjs-bridge/spec.md b/openspec/changes/yjs-undo-redo-manager/specs/yjs-bridge/spec.md index f696b5b6..7f651e2e 100644 --- a/openspec/changes/yjs-undo-redo-manager/specs/yjs-bridge/spec.md +++ b/openspec/changes/yjs-undo-redo-manager/specs/yjs-bridge/spec.md @@ -44,6 +44,43 @@ The bridge SHALL prevent echo loops where a local MMP event writes to Y.Doc, whi - **THEN** the Y.Doc observer SHALL detect `transaction.origin === yUndoManager` and SHALL apply the changes to MMP - **AND** MMP SHALL re-render the affected nodes +### Requirement: Coordinate preservation on node restore +When the Y.Doc observer applies a node-add to MMP (from undo/redo or remote sync), and the node's Y.Map contains non-zero coordinates, the bridge SHALL ensure those coordinates are used as the node's position. The MMP layer SHALL NOT recalculate positions for nodes that already have stored coordinates from Y.Doc. + +**Bug reference:** `nodes.ts:124` unconditionally calls `calculateCoordinates(node)`, overwriting the coordinates passed in from Y.Doc with freshly calculated positions. This causes restored nodes to appear at "append to bottom of siblings" positions rather than their original locations. + +#### Scenario: Undo of delete preserves original coordinates +- **WHEN** a node at coordinates `{x: 200, y: -120}` is deleted and the user triggers undo +- **AND** the Y.UndoManager restores the node to Y.Doc with its original coordinates +- **THEN** the bridge SHALL pass the coordinates from Y.Doc to MMP +- **AND** MMP SHALL render the node at `{x: 200, y: -120}`, NOT at a recalculated position + +#### Scenario: Undo of subtree delete preserves all node coordinates +- **WHEN** a parent node with descendants is deleted (single transaction) and the user triggers undo +- **THEN** ALL restored nodes (parent and descendants) SHALL retain their original coordinates from Y.Doc +- **AND** no node SHALL be repositioned by `calculateCoordinates()` + +#### Scenario: Remote node-add with coordinates preserves position +- **WHEN** a remote client creates a node with specific coordinates +- **AND** the Y.Doc sync delivers the node with those coordinates +- **THEN** the bridge SHALL apply the node to MMP at the coordinates from Y.Doc + +### Requirement: Parent-first ordering on batch node restore +When a Y.Doc transaction adds multiple nodes (e.g., undo of a subtree delete), the observer SHALL process parent nodes before their children. This ensures that when `addNode()` is called for a child, the parent already exists in MMP's node map and can be resolved. + +**Bug reference:** `keysChanged.forEach` in `handleTopLevelNodeChanges` (`map-sync.service.ts:1557`) iterates in Y.Map internal order, which may not be parent-first. A child node processed before its parent causes `getNode(parentId)` to return `undefined`, corrupting the parent reference and position. + +#### Scenario: Subtree restore processes parent before children +- **GIVEN** a parent node `A` with child `B` and grandchild `C` were deleted in one transaction +- **WHEN** the user triggers undo and all three nodes are restored in one transaction +- **THEN** the observer SHALL add `A` to MMP before `B`, and `B` before `C` +- **AND** each child SHALL have a valid parent reference in MMP at the time of insertion + +#### Scenario: Flat siblings restored in any order +- **GIVEN** two sibling nodes `B` and `C` (both children of `A`) were deleted +- **WHEN** the undo restores them +- **THEN** `A` SHALL be added first (parent-first), but `B` and `C` MAY be added in any order relative to each other + ## REMOVED Requirements ### Requirement: Local undo/redo stays local diff --git a/openspec/changes/yjs-undo-redo-manager/specs/yjs-undo/spec.md b/openspec/changes/yjs-undo-redo-manager/specs/yjs-undo/spec.md index 4b351018..05e62485 100644 --- a/openspec/changes/yjs-undo-redo-manager/specs/yjs-undo/spec.md +++ b/openspec/changes/yjs-undo-redo-manager/specs/yjs-undo/spec.md @@ -69,6 +69,29 @@ When `yjsEnabled` is true, the `MapSyncService` SHALL expose public `undo()` and - **WHEN** the user presses undo but the UndoManager's undo stack is empty - **THEN** the `undo()` call SHALL be a no-op (Y.UndoManager handles this gracefully) +### Requirement: Full property fidelity on undo/redo of delete +When a node (or subtree) is deleted and the deletion is undone, ALL node properties stored in Y.Doc SHALL be restored exactly — including coordinates, colors, font, image, link, parent reference, k value, locked state, and detached state. The redo of such an undo (re-delete) and subsequent undo (re-restore) SHALL also preserve full property fidelity. + +#### Scenario: Undo of single node delete restores all properties +- **GIVEN** a node exists with `coordinates: {x: 200, y: -120}`, `colors: {name: '#000', background: '#fff', branch: '#333'}`, and `k: -1` +- **WHEN** the node is deleted and the user triggers undo +- **THEN** the Y.Doc SHALL contain the restored node with ALL original properties intact +- **AND** `coordinates` SHALL be `{x: 200, y: -120}` (not recalculated) +- **AND** `k` SHALL be `-1` (preserving left/right orientation) + +#### Scenario: Undo of subtree delete restores parent and all descendants +- **GIVEN** node `A` has child `B`, and `B` has child `C` +- **AND** all three nodes have distinct coordinates and properties +- **WHEN** `A` is deleted (which cascades to `B` and `C` in a single transaction) +- **AND** the user triggers undo +- **THEN** the Y.Doc SHALL contain all three nodes with their original properties +- **AND** `B.parent` SHALL be `A.id` and `C.parent` SHALL be `B.id` + +#### Scenario: Multiple undo/redo cycles preserve properties +- **GIVEN** a node exists with specific coordinates and properties +- **WHEN** the node is deleted, then undo, then redo (re-delete), then undo (re-restore) +- **THEN** after the final undo, the node's properties in Y.Doc SHALL match the original values exactly + ### Requirement: Reactive canUndo and canRedo state The `MapSyncService` SHALL expose `canUndo$` and `canRedo$` as `Observable` so the toolbar can reactively enable/disable undo/redo buttons when Yjs is active. diff --git a/openspec/changes/yjs-undo-redo-manager/tasks.md b/openspec/changes/yjs-undo-redo-manager/tasks.md index e0b73450..f3ae9605 100644 --- a/openspec/changes/yjs-undo-redo-manager/tasks.md +++ b/openspec/changes/yjs-undo-redo-manager/tasks.md @@ -1,55 +1,73 @@ ## 1. Add transaction origins to all Y.Doc write operations -- [ ] 1.1 Wrap `writeNodeCreateToYDoc` in `yDoc.transact(() => { ... }, 'local')` — currently writes without a transaction -- [ ] 1.2 Wrap `writeNodeUpdateToYDoc` in `yDoc.transact(() => { ... }, 'local')` — currently a bare `yNode.set()` call -- [ ] 1.3 Add `'local'` origin to `writeNodeRemoveFromYDoc` — already uses `yDoc.transact()`, add second argument -- [ ] 1.4 Add `'local'` origin to `writeNodesPasteToYDoc` — already uses `yDoc.transact()`, add second argument -- [ ] 1.5 Wrap `writeMapOptionsToYDoc` in `yDoc.transact(() => { ... }, 'local')` — currently individual `set()` calls -- [ ] 1.6 Change `writeImportToYDoc` to use `'import'` origin — already uses `yDoc.transact()`, change to untracked origin -- [ ] 1.7 Write unit tests verifying each write method uses the correct transaction origin +- [x] 1.1 Wrap `writeNodeCreateToYDoc` in `yDoc.transact(() => { ... }, 'local')` — currently writes without a transaction +- [x] 1.2 Wrap `writeNodeUpdateToYDoc` in `yDoc.transact(() => { ... }, 'local')` — currently a bare `yNode.set()` call +- [x] 1.3 Add `'local'` origin to `writeNodeRemoveFromYDoc` — already uses `yDoc.transact()`, add second argument +- [x] 1.4 Add `'local'` origin to `writeNodesPasteToYDoc` — already uses `yDoc.transact()`, add second argument +- [x] 1.5 Wrap `writeMapOptionsToYDoc` in `yDoc.transact(() => { ... }, 'local')` — currently individual `set()` calls +- [x] 1.6 Change `writeImportToYDoc` to use `'import'` origin — already uses `yDoc.transact()`, change to untracked origin +- [x] 1.7 Write unit tests verifying each write method uses the correct transaction origin ## 2. Add Y.UndoManager lifecycle to MapSyncService -- [ ] 2.1 Add `private yUndoManager: Y.UndoManager | null = null` field to `MapSyncService` -- [ ] 2.2 Create `initYjsUndoManager()` method that instantiates `Y.UndoManager` on `nodesMap` with `trackedOrigins: new Set(['local'])` -- [ ] 2.3 Call `initYjsUndoManager()` in `handleFirstYjsSync()` AFTER `loadMapFromYDoc()` and `setupYjsNodesObserver()` complete -- [ ] 2.4 Add `yUndoManager.destroy()` to `resetYjs()` before `yDoc.destroy()` -- [ ] 2.5 Write unit tests: UndoManager created after first sync, destroyed on reset, undo stack empty after init +- [x] 2.1 Add `private yUndoManager: Y.UndoManager | null = null` field to `MapSyncService` +- [x] 2.2 Create `initYjsUndoManager()` method that instantiates `Y.UndoManager` on `nodesMap` with `trackedOrigins: new Set(['local'])` +- [x] 2.3 Call `initYjsUndoManager()` in `handleFirstYjsSync()` AFTER `loadMapFromYDoc()` and `setupYjsNodesObserver()` complete +- [x] 2.4 Add `yUndoManager.destroy()` to `resetYjs()` before `yDoc.destroy()` +- [x] 2.5 Write unit tests: UndoManager created after first sync, destroyed on reset, undo stack empty after init ## 3. Update echo prevention in Y.Doc observers -- [ ] 3.1 Change `setupYjsNodesObserver` filter from `if (transaction.local) return` to `if (transaction.local && transaction.origin !== this.yUndoManager) return` -- [ ] 3.2 Apply same origin-based filter in `setupYjsMapOptionsObserver` if applicable -- [ ] 3.3 Write unit tests: local edit skipped, remote edit applied, UndoManager-originated edit applied to MMP +- [x] 3.1 Change `setupYjsNodesObserver` filter from `if (transaction.local) return` to `if (transaction.local && transaction.origin !== this.yUndoManager) return` +- [x] 3.2 Apply same origin-based filter in `setupYjsMapOptionsObserver` if applicable +- [x] 3.3 Write unit tests: local edit skipped, remote edit applied, UndoManager-originated edit applied to MMP ## 4. Expose undo/redo methods and reactive state -- [ ] 4.1 Add public `undo()` method that calls `this.yUndoManager?.undo()` -- [ ] 4.2 Add public `redo()` method that calls `this.yUndoManager?.redo()` -- [ ] 4.3 Add `canUndo$` and `canRedo$` as `BehaviorSubject` fields, exposed as `Observable` -- [ ] 4.4 Subscribe to UndoManager `stack-item-added` and `stack-item-popped` events to update `canUndo$`/`canRedo$` -- [ ] 4.5 Clean up UndoManager event listeners in `resetYjs()` -- [ ] 4.6 Write unit tests: canUndo$/canRedo$ emit correctly after edit, undo, redo, and stack empty +- [x] 4.1 Add public `undo()` method that calls `this.yUndoManager?.undo()` +- [x] 4.2 Add public `redo()` method that calls `this.yUndoManager?.redo()` +- [x] 4.3 Add `canUndo$` and `canRedo$` as `BehaviorSubject` fields, exposed as `Observable` +- [x] 4.4 Subscribe to UndoManager `stack-item-added` and `stack-item-popped` events to update `canUndo$`/`canRedo$` +- [x] 4.5 Clean up UndoManager event listeners in `resetYjs()` +- [x] 4.6 Write unit tests: canUndo$/canRedo$ emit correctly after edit, undo, redo, and stack empty ## 5. Remove diff-based undo bridge -- [ ] 5.1 Remove `setupYjsUndoRedoHandlers()` method and its call site in `setupYjsMmpEventHandlers()` -- [ ] 5.2 Remove `writeUndoRedoDiffToYDoc()`, `writeAddedNodesToYDoc()`, `writeUpdatedNodesToYDoc()`, `writeDeletedNodesFromYDoc()` methods -- [ ] 5.3 Remove unused imports (`MapDiff`, `SnapshotChanges` from `@mmp/map/handlers/history`) if no longer referenced -- [ ] 5.4 Update existing unit tests that reference the removed methods +- [x] 5.1 Remove `setupYjsUndoRedoHandlers()` method and its call site in `setupYjsMmpEventHandlers()` +- [x] 5.2 Remove `writeUndoRedoDiffToYDoc()`, `writeAddedNodesToYDoc()`, `writeUpdatedNodesToYDoc()`, `writeDeletedNodesFromYDoc()` methods +- [x] 5.3 Remove unused imports (`MapDiff`, `SnapshotChanges` from `@mmp/map/handlers/history`) if no longer referenced — kept, still used in Socket.io path +- [x] 5.4 Update existing unit tests that reference the removed methods — no tests referenced removed methods ## 6. Update toolbar to route undo/redo conditionally -- [ ] 6.1 Inject `MapSyncService` and `SettingsService` into toolbar component (check if already available) -- [ ] 6.2 Add `yjsEnabled` property to toolbar, read from `SettingsService.getCachedSystemSettings().featureFlags.yjs` -- [ ] 6.3 Update undo button `(click)` to call `mapSyncService.undo()` when `yjsEnabled`, else `mmpService.undo()` -- [ ] 6.4 Update redo button `(click)` to call `mapSyncService.redo()` when `yjsEnabled`, else `mmpService.redo()` -- [ ] 6.5 Update `canUndoRedo` getter: when `yjsEnabled`, subscribe to `canUndo$`/`canRedo$`; when not, use existing `mmpService.history()` check -- [ ] 6.6 Write unit tests for toolbar conditional routing (both Yjs and non-Yjs paths) +- [x] 6.1 Inject `MapSyncService` and `SettingsService` into toolbar component (check if already available) +- [x] 6.2 Add `yjsEnabled` property to toolbar, read from `SettingsService.getCachedSystemSettings().featureFlags.yjs` +- [x] 6.3 Update undo button `(click)` to call `mapSyncService.undo()` when `yjsEnabled`, else `mmpService.undo()` +- [x] 6.4 Update redo button `(click)` to call `mapSyncService.redo()` when `yjsEnabled`, else `mmpService.redo()` +- [x] 6.5 Update `canUndoRedo` getter: when `yjsEnabled`, subscribe to `canUndo$`/`canRedo$`; when not, use existing `mmpService.history()` check +- [x] 6.6 Write unit tests for toolbar conditional routing (both Yjs and non-Yjs paths) ## 7. Integration testing -- [ ] 7.1 Write integration test: create node, undo reverses it, redo restores it -- [ ] 7.2 Write integration test: undo after import is no-op (import not captured) -- [ ] 7.3 Write integration test: undo/redo button disabled state matches UndoManager stack -- [ ] 7.4 Verify existing Playwright e2e tests pass (undo/redo toolbar buttons still functional) +- [x] 7.1 Write integration test: create node, undo reverses it, redo restores it +- [x] 7.2 Write integration test: undo after import is no-op (import not captured) +- [x] 7.3 Write integration test: undo/redo button disabled state matches UndoManager stack +- [x] 7.4 Verify existing Playwright e2e tests pass (undo/redo toolbar buttons still functional) + +## 8. Fix coordinate preservation in `addNode()` (MMP) + +- [x] 8.1 Make `calculateCoordinates()` conditional at `nodes.ts:124` — skip when incoming properties already contain valid (non-zero) coordinates +- [x] 8.2 Write unit tests: node added with existing coordinates preserves them; node added without coordinates gets calculated positions; root node at (0,0) is not recalculated +- [x] 8.3 Verify `applyCoordinatesToMapSnapshot()` still works correctly (uses same pattern) — confirmed: already uses `if (!node.coordinates)` guard, unaffected by addNode change + +## 9. Fix parent-first ordering in observer batch processing + +- [x] 9.1 In `handleTopLevelNodeChanges`, collect all added keys from `keysChanged`, then sort parent-first using `sortParentFirst()` before calling `applyRemoteNodeAdd()` +- [x] 9.2 Write unit test: when a parent and child are added in a single transaction, parent is processed before child regardless of Y.Map iteration order + +## 10. Undo/redo coordinate preservation tests + +- [x] 10.1 Write integration test: delete node, undo restores node at original coordinates (not recalculated) +- [x] 10.2 Write integration test: delete subtree (parent + descendants), undo restores all nodes at original coordinates +- [x] 10.3 Write integration test: delete subtree, undo, redo, undo — coordinates preserved across multiple cycles +- [x] 10.4 Write integration test: delete subtree, undo, redo, undo — always operates on whole branch (undo stack not fragmented into individual node operations) diff --git a/teammapper-backend/src/map/controllers/maps.controller.spec.ts b/teammapper-backend/src/map/controllers/maps.controller.spec.ts index 87c8440a..22d3462d 100644 --- a/teammapper-backend/src/map/controllers/maps.controller.spec.ts +++ b/teammapper-backend/src/map/controllers/maps.controller.spec.ts @@ -1,8 +1,6 @@ import { Test, TestingModule } from '@nestjs/testing' import MapsController from './maps.controller' import { MapsService } from '../services/maps.service' -import { YjsDocManagerService } from '../services/yjs-doc-manager.service' -import { YjsGateway } from './yjs-gateway.service' import { NotFoundException } from '@nestjs/common' import { MmpMap } from '../entities/mmpMap.entity' import { IMmpClientMap, IMmpClientPrivateMap, Request } from '../types' @@ -35,14 +33,6 @@ describe('MapsController', () => { getMapsOfUser: jest.fn(), }, }, - { - provide: YjsDocManagerService, - useValue: { destroyDoc: jest.fn() }, - }, - { - provide: YjsGateway, - useValue: { closeConnectionsForMap: jest.fn() }, - }, ], }).compile() @@ -121,7 +111,7 @@ describe('MapsController', () => { .spyOn(mapsService, 'exportMapToClient') .mockRejectedValueOnce(new MalformedUUIDError('MalformedUUIDError')) - expect(mapsController.findOne(invalidMapId)).rejects.toThrow( + await expect(mapsController.findOne(invalidMapId)).rejects.toThrow( NotFoundException ) }) diff --git a/teammapper-backend/src/map/controllers/maps.controller.ts b/teammapper-backend/src/map/controllers/maps.controller.ts index d372cdf4..a6ade86a 100644 --- a/teammapper-backend/src/map/controllers/maps.controller.ts +++ b/teammapper-backend/src/map/controllers/maps.controller.ts @@ -8,6 +8,8 @@ import { Param, Post, Logger, + Optional, + Inject, } from '@nestjs/common' import { MapsService } from '../services/maps.service' import { YjsDocManagerService } from '../services/yjs-doc-manager.service' @@ -22,15 +24,18 @@ import { } from '../types' import MalformedUUIDError from '../services/uuid.error' import { EntityNotFoundError } from 'typeorm' -import configService from '../../config.service' @Controller('api/maps') export default class MapsController { private readonly logger = new Logger(MapsController.name) constructor( private mapsService: MapsService, - private yjsDocManager: YjsDocManagerService, - private yjsGateway: YjsGateway + @Optional() + @Inject(YjsDocManagerService) + private yjsDocManager?: YjsDocManagerService, + @Optional() + @Inject(YjsGateway) + private yjsGateway?: YjsGateway ) {} @Get(':id') @@ -67,7 +72,7 @@ export default class MapsController { ): Promise { const mmpMap = await this.mapsService.findMap(mapId) if (mmpMap && mmpMap.adminId === body.adminId) { - if (configService.isYjsEnabled()) { + if (this.yjsGateway && this.yjsDocManager) { this.yjsGateway.closeConnectionsForMap(mapId) this.yjsDocManager.destroyDoc(mapId) } diff --git a/teammapper-backend/src/map/controllers/yjs-gateway.service.spec.ts b/teammapper-backend/src/map/controllers/yjs-gateway.service.spec.ts index c3c81dc0..4c376014 100644 --- a/teammapper-backend/src/map/controllers/yjs-gateway.service.spec.ts +++ b/teammapper-backend/src/map/controllers/yjs-gateway.service.spec.ts @@ -15,25 +15,13 @@ import { WS_CLOSE_MAP_NOT_FOUND, WS_CLOSE_TRY_AGAIN, CONNECTION_SETUP_TIMEOUT_MS, - encodeSyncStep1Message, + MESSAGE_SYNC, + MESSAGE_WRITE_ACCESS, encodeSyncUpdateMessage, } from '../utils/yjsProtocol' - -jest.mock('../../config.service', () => ({ - __esModule: true, - default: { - isYjsEnabled: jest.fn(() => true), - }, -})) - -// Minimal interface to call private methods in tests -interface ConnectionHandler { - handleConnection(ws: MockWs, req: IncomingMessage): Promise -} - -interface HeartbeatRunner { - runHeartbeat(): void -} +import * as syncProtocol from 'y-protocols/sync' +import * as encoding from 'lib0/encoding' +import * as decoding from 'lib0/decoding' const createMockMap = (secret: string | null = 'test-secret'): MmpMap => { const map = new MmpMap() @@ -103,12 +91,17 @@ const createMockRequest = ( } as unknown as IncomingMessage } +// Triggers the private handleConnection method — the WebSocket 'connection' +// event is the natural entry point and cannot be reached through public API +// in a unit test without a real HTTP server. const connectClient = async ( gateway: YjsGateway, ws: MockWs, req: IncomingMessage ) => { - const handler = gateway as unknown as ConnectionHandler + const handler = gateway as unknown as { + handleConnection(ws: MockWs, req: IncomingMessage): Promise + } await handler.handleConnection(ws, req) } @@ -174,8 +167,11 @@ describe('YjsGateway', () => { jest.restoreAllMocks() }) - describe('connection with valid map ID', () => { - it('creates doc and notifies client count', async () => { + // ─── Connection setup ────────────────────────────────────── + + describe('connection setup', () => { + it('syncs full doc state to client on connection', async () => { + doc.getMap('nodes').set('node-1', new Y.Map()) mapsService.findMap.mockResolvedValue(createMockMap()) const ws = createMockWs() @@ -185,11 +181,21 @@ describe('YjsGateway', () => { createMockRequest('map-1', 'test-secret') ) - expect(docManager.getOrCreateDoc).toHaveBeenCalledWith('map-1') - expect(docManager.notifyClientCount).toHaveBeenCalledWith('map-1', 1) + const clientDoc = new Y.Doc() + for (const call of ws.send.mock.calls) { + const data = new Uint8Array(call[0] as Buffer) + const decoder = decoding.createDecoder(data) + if (decoding.readVarUint(decoder) === MESSAGE_SYNC) { + const encoder = encoding.createEncoder() + syncProtocol.readSyncMessage(decoder, encoder, clientDoc, null) + } + } + + expect(clientDoc.getMap('nodes').has('node-1')).toBe(true) + clientDoc.destroy() }) - it('sends sync message on connection', async () => { + it('sends write-access before sync messages', async () => { mapsService.findMap.mockResolvedValue(createMockMap()) const ws = createMockWs() @@ -199,13 +205,20 @@ describe('YjsGateway', () => { createMockRequest('map-1', 'test-secret') ) - expect(ws.send).toHaveBeenCalled() + const messageTypes = ws.send.mock.calls.map((call) => { + const data = new Uint8Array(call[0] as Buffer) + return decoding.readVarUint(decoding.createDecoder(data)) + }) + const writeIdx = messageTypes.indexOf(MESSAGE_WRITE_ACCESS) + const syncIdx = messageTypes.indexOf(MESSAGE_SYNC) + + expect(writeIdx).toBeGreaterThanOrEqual(0) + expect(writeIdx).toBeLessThan(syncIdx) }) - }) - describe('connection with invalid map ID', () => { - it('closes connection when map not found', async () => { + it('closes and cleans up when map not found', async () => { mapsService.findMap.mockResolvedValue(null) + limiter.getClientIp.mockReturnValue('10.0.0.1') const ws = createMockWs() await connectClient(gateway, ws, createMockRequest('nonexistent')) @@ -214,30 +227,11 @@ describe('YjsGateway', () => { WS_CLOSE_MAP_NOT_FOUND, 'Map not found' ) - }) - - it('does not create doc when map not found', async () => { - mapsService.findMap.mockResolvedValue(null) - const ws = createMockWs() - - await connectClient(gateway, ws, createMockRequest('nonexistent')) - - expect(docManager.getOrCreateDoc).not.toHaveBeenCalled() - }) - - it('releases limiter slot when map not found', async () => { - mapsService.findMap.mockResolvedValue(null) - limiter.getClientIp.mockReturnValue('10.0.0.1') - const ws = createMockWs() - - await connectClient(gateway, ws, createMockRequest('nonexistent')) - expect(limiter.releaseConnection).toHaveBeenCalledWith('10.0.0.1') }) - }) - describe('connection with missing mapId parameter', () => { - it('closes connection with error', async () => { + it('closes and cleans up when mapId missing', async () => { + limiter.getClientIp.mockReturnValue('10.0.0.1') const ws = createMockWs() await connectClient(gateway, ws, createMockRequest(null)) @@ -246,38 +240,35 @@ describe('YjsGateway', () => { WS_CLOSE_MISSING_PARAM, 'Missing mapId' ) - }) - - it('releases limiter slot when mapId missing', async () => { - limiter.getClientIp.mockReturnValue('10.0.0.1') - const ws = createMockWs() - - await connectClient(gateway, ws, createMockRequest(null)) - expect(limiter.releaseConnection).toHaveBeenCalledWith('10.0.0.1') }) - }) - describe('read-only client cannot write', () => { - it('allows sync step 1 from read-only client', async () => { - mapsService.findMap.mockResolvedValue(createMockMap('secret-123')) + it('closes with 1013 when setup exceeds timeout', async () => { + jest.useFakeTimers() + mapsService.findMap.mockReturnValue(new Promise(() => {})) const ws = createMockWs() - await connectClient( + const promise = connectClient( gateway, ws, - createMockRequest('map-1', 'wrong-secret') + createMockRequest('map-1', 'test-secret') ) - ws.send.mockClear() - - const clientDoc = new Y.Doc() - ws._triggerMessage(encodeSyncStep1Message(clientDoc)) + jest.advanceTimersByTime(CONNECTION_SETUP_TIMEOUT_MS + 1) + await promise - expect(ws.send).toHaveBeenCalled() - clientDoc.destroy() + expect(ws.close).toHaveBeenCalledWith( + WS_CLOSE_TRY_AGAIN, + 'Connection setup timeout' + ) + expect(limiter.releaseConnection).toHaveBeenCalledWith('127.0.0.1') + jest.useRealTimers() }) + }) + + // ─── Write access control ────────────────────────────────── - it('drops sync update messages from read-only client', async () => { + describe('write access control', () => { + it('drops writes from read-only client', async () => { mapsService.findMap.mockResolvedValue(createMockMap('secret-123')) const ws = createMockWs() @@ -286,72 +277,56 @@ describe('YjsGateway', () => { ws, createMockRequest('map-1', 'wrong-secret') ) - doc.getMap('nodes').set('existing', new Y.Map()) const initialSize = doc.getMap('nodes').size - ws.send.mockClear() const clientDoc = new Y.Doc() clientDoc.getMap('nodes').set('new-node', new Y.Map()) - const update = Y.encodeStateAsUpdate(clientDoc) - ws._triggerMessage(encodeSyncUpdateMessage(update)) + ws._triggerMessage( + encodeSyncUpdateMessage(Y.encodeStateAsUpdate(clientDoc)) + ) expect(doc.getMap('nodes').size).toBe(initialSize) clientDoc.destroy() }) - }) - describe('read-write client can write', () => { - it('applies sync messages from read-write client', async () => { + it('applies writes from client with correct secret', async () => { mapsService.findMap.mockResolvedValue(createMockMap('secret-123')) const ws = createMockWs() await connectClient(gateway, ws, createMockRequest('map-1', 'secret-123')) const clientDoc = new Y.Doc() - ws._triggerMessage(encodeSyncStep1Message(clientDoc)) + clientDoc.getMap('nodes').set('new-node', new Y.Map()) + ws._triggerMessage( + encodeSyncUpdateMessage(Y.encodeStateAsUpdate(clientDoc)) + ) - expect(ws.send).toHaveBeenCalled() + expect(doc.getMap('nodes').has('new-node')).toBe(true) clientDoc.destroy() }) - it('allows writes when map has no modification secret', async () => { + it('grants write access when map has no secret', async () => { mapsService.findMap.mockResolvedValue(createMockMap(null)) const ws = createMockWs() await connectClient(gateway, ws, createMockRequest('map-1')) - ws.send.mockClear() const clientDoc = new Y.Doc() clientDoc.getMap('nodes').set('new-node', new Y.Map()) - const update = Y.encodeStateAsUpdate(clientDoc) - ws._triggerMessage(encodeSyncUpdateMessage(update)) + ws._triggerMessage( + encodeSyncUpdateMessage(Y.encodeStateAsUpdate(clientDoc)) + ) expect(doc.getMap('nodes').has('new-node')).toBe(true) clientDoc.destroy() }) }) - describe('connection close handler', () => { - it('notifies doc manager with remaining count on disconnect', async () => { - mapsService.findMap.mockResolvedValue(createMockMap()) - const ws = createMockWs() + // ─── Disconnect handling ─────────────────────────────────── - await connectClient( - gateway, - ws, - createMockRequest('map-1', 'test-secret') - ) - docManager.notifyClientCount.mockClear() - ws._triggerClose() - - // Allow the async notifyClientCount promise to settle - await new Promise((r) => setTimeout(r, 0)) - - expect(docManager.notifyClientCount).toHaveBeenCalledWith('map-1', 0) - }) - - it('passes correct remaining count with multiple clients', async () => { + describe('disconnect handling', () => { + it('decrements client count on disconnect', async () => { mapsService.findMap.mockResolvedValue(createMockMap()) const ws1 = createMockWs() const ws2 = createMockWs() @@ -368,13 +343,12 @@ describe('YjsGateway', () => { ) docManager.notifyClientCount.mockClear() ws1._triggerClose() - await new Promise((r) => setTimeout(r, 0)) expect(docManager.notifyClientCount).toHaveBeenCalledWith('map-1', 1) }) - it('delegates to limiter on connection close', async () => { + it('releases limiter slot on disconnect', async () => { mapsService.findMap.mockResolvedValue(createMockMap()) limiter.getClientIp.mockReturnValue('10.0.0.1') const ws = createMockWs() @@ -388,59 +362,8 @@ describe('YjsGateway', () => { expect(limiter.releaseConnection).toHaveBeenCalledWith('10.0.0.1') }) - }) - describe('map deletion closes connections', () => { - it('closes all WebSocket connections for a map', async () => { - mapsService.findMap.mockResolvedValue(createMockMap()) - const ws1 = createMockWs() - const ws2 = createMockWs() - - await connectClient( - gateway, - ws1, - createMockRequest('map-1', 'test-secret') - ) - await connectClient( - gateway, - ws2, - createMockRequest('map-1', 'test-secret') - ) - - gateway.closeConnectionsForMap('map-1') - - expect(ws1.close).toHaveBeenCalledWith( - WS_CLOSE_MAP_DELETED, - 'Map deleted' - ) - expect(ws2.close).toHaveBeenCalledWith( - WS_CLOSE_MAP_DELETED, - 'Map deleted' - ) - }) - - it('does nothing for non-existent map', () => { - gateway.closeConnectionsForMap('nonexistent') - }) - }) - - describe('WebSocket error handling', () => { - it('terminates connection on error event', async () => { - mapsService.findMap.mockResolvedValue(createMockMap()) - const ws = createMockWs() - - await connectClient( - gateway, - ws, - createMockRequest('map-1', 'test-secret') - ) - - ws._triggerError(new Error('ECONNRESET')) - - expect(ws.terminate).toHaveBeenCalled() - }) - - it('does not crash the server process on error', async () => { + it('survives notifyClientCount errors without crashing', async () => { mapsService.findMap.mockResolvedValue(createMockMap()) const ws = createMockWs() @@ -449,31 +372,15 @@ describe('YjsGateway', () => { ws, createMockRequest('map-1', 'test-secret') ) - - expect(() => ws._triggerError(new Error('write EPIPE'))).not.toThrow() - }) - }) - - describe('ping/pong heartbeat', () => { - const runHeartbeat = (gw: YjsGateway) => - (gw as unknown as HeartbeatRunner).runHeartbeat() - - it('pings on first heartbeat', async () => { - mapsService.findMap.mockResolvedValue(createMockMap()) - const ws = createMockWs() - - await connectClient( - gateway, - ws, - createMockRequest('map-1', 'test-secret') + docManager.notifyClientCount.mockRejectedValue( + new Error('DB connection lost') ) - runHeartbeat(gateway) - - expect(ws.ping).toHaveBeenCalledTimes(1) + expect(() => ws._triggerClose()).not.toThrow() + await new Promise((r) => setTimeout(r, 0)) }) - it('terminates zombie on second heartbeat without pong', async () => { + it('terminates connection on WebSocket error', async () => { mapsService.findMap.mockResolvedValue(createMockMap()) const ws = createMockWs() @@ -482,141 +389,55 @@ describe('YjsGateway', () => { ws, createMockRequest('map-1', 'test-secret') ) - - runHeartbeat(gateway) - runHeartbeat(gateway) + ws._triggerError(new Error('ECONNRESET')) expect(ws.terminate).toHaveBeenCalled() }) - - it('survives after pong response', async () => { - mapsService.findMap.mockResolvedValue(createMockMap()) - const ws = createMockWs() - - await connectClient( - gateway, - ws, - createMockRequest('map-1', 'test-secret') - ) - - runHeartbeat(gateway) - ws._triggerPong() - runHeartbeat(gateway) - - expect(ws.terminate).not.toHaveBeenCalled() - }) - - it('pings on each heartbeat cycle', async () => { - mapsService.findMap.mockResolvedValue(createMockMap()) - const ws = createMockWs() - - await connectClient( - gateway, - ws, - createMockRequest('map-1', 'test-secret') - ) - - runHeartbeat(gateway) - ws._triggerPong() - runHeartbeat(gateway) - - expect(ws.ping).toHaveBeenCalledTimes(2) - }) - - it('delegates rate window cleanup to limiter', async () => { - runHeartbeat(gateway) - - expect(limiter.cleanupExpiredRateWindows).toHaveBeenCalled() - }) - - it('clears heartbeat interval on shutdown', () => { - gateway.onModuleInit() - gateway.onModuleDestroy() - - // After destroy, no further heartbeat runs - // (verified by no errors thrown after cleanup) - }) }) - describe('connection setup timeout', () => { - beforeEach(() => { - jest.useFakeTimers() - }) - - afterEach(() => { - jest.useRealTimers() - }) - - it('closes with 1013 when setup exceeds timeout', async () => { - mapsService.findMap.mockReturnValue(new Promise(() => {})) - const ws = createMockWs() - - const promise = connectClient( - gateway, - ws, - createMockRequest('map-1', 'test-secret') - ) + // ─── closeConnectionsForMap (public API) ─────────────────── - jest.advanceTimersByTime(CONNECTION_SETUP_TIMEOUT_MS + 1) - await promise - - expect(ws.close).toHaveBeenCalledWith( - WS_CLOSE_TRY_AGAIN, - 'Connection setup timeout' - ) - expect(limiter.releaseConnection).toHaveBeenCalledWith('127.0.0.1') - }) - - it('completes normally when setup finishes within timeout', async () => { + describe('closeConnectionsForMap', () => { + it('closes all connections for the specified map', async () => { mapsService.findMap.mockResolvedValue(createMockMap()) - const ws = createMockWs() + const ws1 = createMockWs() + const ws2 = createMockWs() await connectClient( gateway, - ws, + ws1, createMockRequest('map-1', 'test-secret') ) - - expect(ws.close).not.toHaveBeenCalledWith( - WS_CLOSE_TRY_AGAIN, - expect.any(String) - ) - expect(docManager.getOrCreateDoc).toHaveBeenCalledWith('map-1') - }) - - it('cancels timer on early completion', async () => { - mapsService.findMap.mockResolvedValue(createMockMap()) - const ws = createMockWs() - await connectClient( gateway, - ws, + ws2, createMockRequest('map-1', 'test-secret') ) - jest.advanceTimersByTime(15_000) - await Promise.resolve() + gateway.closeConnectionsForMap('map-1') - expect(ws.close).not.toHaveBeenCalledWith( - WS_CLOSE_TRY_AGAIN, - expect.any(String) + expect(ws1.close).toHaveBeenCalledWith( + WS_CLOSE_MAP_DELETED, + 'Map deleted' + ) + expect(ws2.close).toHaveBeenCalledWith( + WS_CLOSE_MAP_DELETED, + 'Map deleted' ) }) - }) - describe('multiple maps isolation', () => { - it('connections to different maps are independent', async () => { + it('does not affect connections to other maps', async () => { const map1 = createMockMap() const map2 = createMockMap() map2.id = 'map-2' - const doc2 = new Y.Doc() - mapsService.findMap.mockImplementation(async (id: string) => { - return id === 'map-1' ? map1 : map2 - }) - docManager.getOrCreateDoc.mockImplementation(async (id: string) => { - return id === 'map-1' ? doc : doc2 - }) + + mapsService.findMap.mockImplementation(async (id: string) => + id === 'map-1' ? map1 : map2 + ) + docManager.getOrCreateDoc.mockImplementation(async (id: string) => + id === 'map-1' ? doc : doc2 + ) const ws1 = createMockWs() const ws2 = createMockWs() @@ -640,64 +461,4 @@ describe('YjsGateway', () => { doc2.destroy() }) }) - - describe('notifyClientCount error handling', () => { - it('logs persistence errors on disconnect without crashing', async () => { - mapsService.findMap.mockResolvedValue(createMockMap()) - const ws = createMockWs() - - await connectClient( - gateway, - ws, - createMockRequest('map-1', 'test-secret') - ) - - // Mock to reject for the close handler call - docManager.notifyClientCount.mockRejectedValue( - new Error('DB connection lost') - ) - - // Close should not throw even when notifyClientCount rejects - expect(() => ws._triggerClose()).not.toThrow() - - // Allow the promise rejection to be caught - await new Promise((r) => setTimeout(r, 0)) - - expect(docManager.notifyClientCount).toHaveBeenCalledWith('map-1', 0) - }) - }) - - describe('grace timer restoration on setup failure', () => { - it('restores grace timer when notifyClientCount throws', async () => { - mapsService.findMap.mockResolvedValue(createMockMap()) - docManager.notifyClientCount.mockRejectedValue( - new Error('Unexpected error') - ) - const ws = createMockWs() - - await connectClient( - gateway, - ws, - createMockRequest('map-1', 'test-secret') - ) - - expect(docManager.restoreGraceTimer).toHaveBeenCalledWith( - 'map-1', - expect.any(Number) - ) - }) - - it('does not restore grace timer on successful setup', async () => { - mapsService.findMap.mockResolvedValue(createMockMap()) - const ws = createMockWs() - - await connectClient( - gateway, - ws, - createMockRequest('map-1', 'test-secret') - ) - - expect(docManager.restoreGraceTimer).not.toHaveBeenCalled() - }) - }) }) diff --git a/teammapper-backend/src/map/controllers/yjs-gateway.service.ts b/teammapper-backend/src/map/controllers/yjs-gateway.service.ts index 703830ba..e6cef004 100644 --- a/teammapper-backend/src/map/controllers/yjs-gateway.service.ts +++ b/teammapper-backend/src/map/controllers/yjs-gateway.service.ts @@ -13,7 +13,6 @@ import * as syncProtocol from 'y-protocols/sync' import * as awarenessProtocol from 'y-protocols/awareness' import * as encoding from 'lib0/encoding' import * as decoding from 'lib0/decoding' -import configService from '../../config.service' import { YjsDocManagerService } from '../services/yjs-doc-manager.service' import { YjsPersistenceService } from '../services/yjs-persistence.service' import { MapsService } from '../services/maps.service' @@ -36,6 +35,7 @@ import { checkWriteAccess, toUint8Array, encodeSyncStep1Message, + encodeSyncStep2Message, encodeSyncUpdateMessage, encodeAwarenessMessage, encodeWriteAccessMessage, @@ -73,11 +73,6 @@ export class YjsGateway implements OnModuleInit, OnModuleDestroy { ) {} onModuleInit(): void { - if (!configService.isYjsEnabled()) { - this.logger.log('Yjs disabled, skipping WebSocket server setup') - return - } - const server = this.httpAdapterHost.httpAdapter.getHttpServer() this.wss = new WebSocketServer({ noServer: true, @@ -341,9 +336,29 @@ export class YjsGateway implements OnModuleInit, OnModuleDestroy { this.handleClose(ws, mapId, awareness) }) + // Sync strategy: we proactively push the full doc state to the client + // rather than waiting for the standard SyncStep1/SyncStep2 handshake. + // + // Why: handleConnection is async (DB lookup, doc creation) so the + // ws.on('message') handler above is registered late. The y-websocket + // client sends its SyncStep1 immediately on open, which may arrive + // before the handler exists and get silently dropped. Without a + // proactive push the client would never receive the doc. + // + // Trade-off: the proactive SyncStep2 always sends the full doc state + // (not a diff), so reconnecting clients receive redundant data. For + // mind-map-sized documents this is negligible. A message-buffering + // approach would be more efficient for large documents. + // + // Message order: + // 1. Write-access — client knows its permissions before sync fires + // 2. SyncStep1 — server state vector (triggers client's SyncStep2) + // 3. SyncStep2 — full doc state for immediate hydration + // 4. Awareness — existing cursors/presence + this.send(ws, encodeWriteAccessMessage(writable)) this.send(ws, encodeSyncStep1Message(doc)) + this.send(ws, encodeSyncStep2Message(doc)) this.sendAwarenessStates(ws, awareness) - this.send(ws, encodeWriteAccessMessage(writable)) } private handleMessage( diff --git a/teammapper-backend/src/map/map.module.ts b/teammapper-backend/src/map/map.module.ts index 2f4d142f..23dbc650 100644 --- a/teammapper-backend/src/map/map.module.ts +++ b/teammapper-backend/src/map/map.module.ts @@ -18,20 +18,19 @@ import cookieParser from 'cookie-parser' import { PersonIdMiddleware } from '../auth/person-id.middleware' import configService from '../config.service' -// When Yjs is enabled, the Socket.io gateway (MapsGateway) must be excluded -// because both cannot bind to the same HTTP upgrade path simultaneously. -const baseProviders: Provider[] = [ - MapsService, +// When Yjs is enabled, the Yjs providers replace the Socket.io MapsGateway. +// Both cannot bind to the same HTTP upgrade path simultaneously. +const baseProviders: Provider[] = [MapsService, TasksService, AiService] + +const yjsProviders: Provider[] = [ YjsDocManagerService, YjsPersistenceService, WsConnectionLimiterService, YjsGateway, - TasksService, - AiService, ] const mapProviders: Provider[] = configService.isYjsEnabled() - ? baseProviders + ? [...baseProviders, ...yjsProviders] : [...baseProviders, MapsGateway] @Module({ diff --git a/teammapper-backend/src/map/utils/yjsProtocol.ts b/teammapper-backend/src/map/utils/yjsProtocol.ts index eff8f8dc..4837898e 100644 --- a/teammapper-backend/src/map/utils/yjsProtocol.ts +++ b/teammapper-backend/src/map/utils/yjsProtocol.ts @@ -51,6 +51,16 @@ export const encodeSyncStep1Message = (doc: Y.Doc): Uint8Array => { return encoding.toUint8Array(encoder) } +// Encodes a SyncStep2 message (full doc state) for immediate client hydration. +// Called without a client state vector, so it encodes the entire doc — not a +// diff. This is intentional: see setupSync() in yjs-gateway for rationale. +export const encodeSyncStep2Message = (doc: Y.Doc): Uint8Array => { + const encoder = encoding.createEncoder() + encoding.writeVarUint(encoder, MESSAGE_SYNC) + syncProtocol.writeSyncStep2(encoder, doc) + return encoding.toUint8Array(encoder) +} + // Encodes a sync update message for broadcasting doc changes export const encodeSyncUpdateMessage = (update: Uint8Array): Uint8Array => { const encoder = encoding.createEncoder() diff --git a/teammapper-backend/test/jest-setup.ts b/teammapper-backend/test/jest-setup.ts index 644679e1..c36664bd 100644 --- a/teammapper-backend/test/jest-setup.ts +++ b/teammapper-backend/test/jest-setup.ts @@ -1,7 +1,7 @@ // Disable Yjs feature flag so that MapModule includes the legacy Socket.io -// MapsGateway in its providers. When YJS_ENABLED is true, MapModule excludes -// MapsGateway because both gateways cannot bind to the same HTTP upgrade path -// simultaneously in production. The e2e tests (app.e2e-spec, map-operations-error.e2e-spec) -// rely on the Socket.io gateway for join/addNodes/updateNode events, so it must -// be registered. Unit tests that need Yjs enabled mock configService directly. +// MapsGateway and excludes all Yjs providers (YjsGateway, YjsDocManagerService, +// YjsPersistenceService, WsConnectionLimiterService). The e2e tests +// (app.e2e-spec, map-operations-error.e2e-spec) rely on the Socket.io gateway +// for join/addNodes/updateNode events. Unit tests that need Yjs enabled mock +// configService directly. process.env.YJS_ENABLED = 'false' diff --git a/teammapper-frontend/mmp/src/map/handlers/nodes.ts b/teammapper-frontend/mmp/src/map/handlers/nodes.ts index 905d4e0f..c7295cd6 100644 --- a/teammapper-frontend/mmp/src/map/handlers/nodes.ts +++ b/teammapper-frontend/mmp/src/map/handlers/nodes.ts @@ -121,7 +121,9 @@ export default class Nodes { this.counter++; - node.coordinates = this.calculateCoordinates(node); + if (!properties.coordinates?.x && !properties.coordinates?.y && !node.isRoot) { + node.coordinates = this.calculateCoordinates(node); + } this.map.draw.update(); @@ -597,13 +599,11 @@ export default class Nodes { node: Node | ExportNodeProperties, rootNode?: Node | ExportNodeProperties ): boolean | undefined { - // isRoot exists only on Node type, so type guard is needed - if ('isRoot' in node && node.isRoot) { + if (node.isRoot) { return; } - // For Node type, use this.getRoot() if rootNode not provided - const root = rootNode ?? ('isRoot' in node ? this.getRoot() : undefined); + const root = rootNode ?? (node instanceof Node ? this.getRoot() : undefined); if (!root) { return; } diff --git a/teammapper-frontend/src/app/core/services/map-sync/map-sync.service.spec.ts b/teammapper-frontend/src/app/core/services/map-sync/map-sync.service.spec.ts index c219b4ae..57bb1ddb 100644 --- a/teammapper-frontend/src/app/core/services/map-sync/map-sync.service.spec.ts +++ b/teammapper-frontend/src/app/core/services/map-sync/map-sync.service.spec.ts @@ -553,6 +553,123 @@ describe('write access message parsing', () => { }); }); +describe('parseWriteAccessMessage edit mode behavior', () => { + let service: MapSyncService; + let settingsService: jest.Mocked; + + interface ServiceInternals { + parseWriteAccessMessage: (e: MessageEvent) => void; + yjsWritable: boolean; + yjsSynced: boolean; + } + + beforeEach(() => { + const mmpService = { + new: jest.fn(), + selectNode: jest.fn(), + getRootNode: jest + .fn() + .mockReturnValue( + createMockNode({ id: 'root', name: 'Root', isRoot: true }) + ), + on: jest.fn().mockReturnValue({ + subscribe: jest.fn().mockReturnValue({ unsubscribe: jest.fn() }), + }), + updateNode: jest.fn(), + existNode: jest.fn().mockReturnValue(true), + addNodesFromServer: jest.fn(), + removeNode: jest.fn(), + highlightNode: jest.fn(), + exportAsJSON: jest.fn().mockReturnValue([]), + } as unknown as jest.Mocked; + + settingsService = { + getCachedUserSettings: jest.fn().mockReturnValue({ + mapOptions: { rootNode: 'Root' }, + }), + getCachedSystemSettings: jest.fn().mockReturnValue({ + featureFlags: { yjs: false, pictograms: false, ai: false }, + }), + setEditMode: jest.fn(), + } as unknown as jest.Mocked; + + TestBed.configureTestingModule({ + providers: [ + MapSyncService, + { provide: MmpService, useValue: mmpService }, + { provide: HttpService, useValue: { get: jest.fn(), post: jest.fn() } }, + { + provide: StorageService, + useValue: { get: jest.fn(), set: jest.fn() }, + }, + { provide: SettingsService, useValue: settingsService }, + { provide: UtilsService, useValue: createMockUtilsService() }, + { + provide: ToastService, + useValue: { showValidationCorrection: jest.fn() }, + }, + { + provide: DialogService, + useValue: { openCriticalErrorDialog: jest.fn() }, + }, + { + provide: ToastrService, + useValue: { + error: jest.fn(), + success: jest.fn(), + warning: jest.fn(), + }, + }, + ], + }); + + service = TestBed.inject(MapSyncService); + }); + + afterEach(() => { + service.ngOnDestroy(); + }); + + it('does not call setEditMode when not yet synced', () => { + const writeAccessData = new Uint8Array([4, 1]).buffer; + const event = new MessageEvent('message', { data: writeAccessData }); + + (service as unknown as ServiceInternals).parseWriteAccessMessage(event); + + expect(settingsService.setEditMode).not.toHaveBeenCalled(); + }); + + it('calls setEditMode when already synced', () => { + (service as unknown as ServiceInternals).yjsSynced = true; + + const writeAccessData = new Uint8Array([4, 1]).buffer; + const event = new MessageEvent('message', { data: writeAccessData }); + + (service as unknown as ServiceInternals).parseWriteAccessMessage(event); + + expect(settingsService.setEditMode).toHaveBeenCalledWith(true); + }); + + it('stores yjsWritable=false without calling setEditMode when not synced', () => { + const readOnlyData = new Uint8Array([4, 0]).buffer; + const event = new MessageEvent('message', { data: readOnlyData }); + + (service as unknown as ServiceInternals).parseWriteAccessMessage(event); + + expect(settingsService.setEditMode).not.toHaveBeenCalled(); + }); + + it('does not store yjsWritable for non-write-access messages', () => { + const nonWriteAccessData = new Uint8Array([0, 1]).buffer; + const event = new MessageEvent('message', { data: nonWriteAccessData }); + + (service as unknown as ServiceInternals).parseWriteAccessMessage(event); + + expect(settingsService.setEditMode).not.toHaveBeenCalled(); + expect((service as unknown as ServiceInternals).yjsWritable).toBe(false); + }); +}); + describe('Yjs URL building', () => { let querySelectorSpy: jest.SpyInstance; diff --git a/teammapper-frontend/src/app/core/services/map-sync/map-sync.service.ts b/teammapper-frontend/src/app/core/services/map-sync/map-sync.service.ts index 52ae5f4e..3a0c109f 100644 --- a/teammapper-frontend/src/app/core/services/map-sync/map-sync.service.ts +++ b/teammapper-frontend/src/app/core/services/map-sync/map-sync.service.ts @@ -75,8 +75,8 @@ export class MapSyncService implements OnDestroy { private httpService = inject(HttpService); private storageService = inject(StorageService); private settingsService = inject(SettingsService); - utilsService = inject(UtilsService); - toastrService = inject(ToastrService); + private utilsService = inject(UtilsService); + private toastrService = inject(ToastrService); private toastService = inject(ToastService); private dialogService = inject(DialogService); @@ -88,6 +88,13 @@ export class MapSyncService implements OnDestroy { private readonly attachedNodeSubject: BehaviorSubject; // inform other parts of the app about the connection state private readonly connectionStatusSubject: BehaviorSubject; + // Yjs undo/redo state + private readonly canUndoSubject = new BehaviorSubject(false); + private readonly canRedoSubject = new BehaviorSubject(false); + public readonly canUndo$: Observable = + this.canUndoSubject.asObservable(); + public readonly canRedo$: Observable = + this.canRedoSubject.asObservable(); // Socket.io fields (used when yjs feature flag is false) private socket: Socket; @@ -105,6 +112,7 @@ export class MapSyncService implements OnDestroy { private yjsOptionsObserver: Parameters['observe']>[0] | null = null; private yjsAwarenessHandler: (() => void) | null = null; + private yUndoManager: Y.UndoManager | null = null; // Common fields private colorMapping: ClientColorMapping; @@ -245,6 +253,14 @@ export class MapSyncService implements OnDestroy { this.attachMap({ key: cachedMapEntry.key, cachedMap }); } + public undo(): void { + this.yUndoManager?.undo(); + } + + public redo(): void { + this.yUndoManager?.redo(); + } + public updateMapOptions(options?: CachedMapOptions) { if (this.yjsEnabled) { this.writeMapOptionsToYDoc(options); @@ -1165,11 +1181,31 @@ export class MapSyncService implements OnDestroy { this.loadMapFromYDoc(); this.setupYjsNodesObserver(); this.setupYjsMapOptionsObserver(); + this.initYjsUndoManager(); this.setupYjsAwareness(); this.settingsService.setEditMode(this.yjsWritable); this.setConnectionStatusSubject('connected'); } + private initYjsUndoManager(): void { + const nodesMap = this.yDoc.getMap('nodes'); + this.yUndoManager = new Y.UndoManager(nodesMap, { + trackedOrigins: new Set(['local']), + }); + this.setupUndoManagerListeners(); + } + + private setupUndoManagerListeners(): void { + const updateUndoRedoState = () => { + if (!this.yUndoManager) return; + this.canUndoSubject.next(this.yUndoManager.undoStack.length > 0); + this.canRedoSubject.next(this.yUndoManager.redoStack.length > 0); + }; + + this.yUndoManager.on('stack-item-added', updateUndoRedoState); + this.yUndoManager.on('stack-item-popped', updateUndoRedoState); + } + private setupYjsConnectionStatus(): void { this.wsProvider.on( 'status', @@ -1221,13 +1257,23 @@ export class MapSyncService implements OnDestroy { const result = parseWriteAccessBytes(data); if (result !== null) { this.yjsWritable = result; - this.settingsService.setEditMode(this.yjsWritable); + // If sync already completed, apply edit mode immediately + // (defense against message ordering races) + if (this.yjsSynced) { + this.settingsService.setEditMode(result); + } } } private resetYjs(): void { this.unsubscribeYjsListeners(); this.detachYjsObservers(); + if (this.yUndoManager) { + this.yUndoManager.destroy(); + this.yUndoManager = null; + this.canUndoSubject.next(false); + this.canRedoSubject.next(false); + } const provider = this.wsProvider; this.wsProvider = null; if (provider) { @@ -1270,7 +1316,6 @@ export class MapSyncService implements OnDestroy { this.setupYjsCreateHandler(); this.setupYjsSelectionHandlers(); this.setupYjsNodeUpdateHandler(); - this.setupYjsUndoRedoHandlers(); this.setupYjsNodeCreateHandler(); this.setupYjsPasteHandler(); this.setupYjsNodeRemoveHandler(); @@ -1327,26 +1372,6 @@ export class MapSyncService implements OnDestroy { ); } - private setupYjsUndoRedoHandlers(): void { - this.yjsSubscriptions.push( - this.mmpService.on('undo').subscribe((diff?: MapDiff) => { - if (!this.yDoc) return; - this.attachedNodeSubject.next(this.mmpService.selectNode()); - this.updateAttachedMap(); - this.writeUndoRedoDiffToYDoc(diff); - }) - ); - - this.yjsSubscriptions.push( - this.mmpService.on('redo').subscribe((diff?: MapDiff) => { - if (!this.yDoc) return; - this.attachedNodeSubject.next(this.mmpService.selectNode()); - this.updateAttachedMap(); - this.writeUndoRedoDiffToYDoc(diff); - }) - ); - } - private setupYjsNodeCreateHandler(): void { this.yjsSubscriptions.push( this.mmpService @@ -1389,9 +1414,11 @@ export class MapSyncService implements OnDestroy { private writeNodeCreateToYDoc(nodeProps: ExportNodeProperties): void { const nodesMap = this.yDoc.getMap('nodes') as Y.Map>; - const yNode = new Y.Map(); - this.populateYMapFromNodeProps(yNode, nodeProps); - nodesMap.set(nodeProps.id, yNode); + this.yDoc.transact(() => { + const yNode = new Y.Map(); + this.populateYMapFromNodeProps(yNode, nodeProps); + nodesMap.set(nodeProps.id, yNode); + }, 'local'); } private writeNodeUpdateToYDoc(event: NodeUpdateEvent): void { @@ -1399,10 +1426,12 @@ export class MapSyncService implements OnDestroy { const yNode = nodesMap.get(event.nodeProperties.id); if (!yNode) return; - const topLevelKey = NodePropertyMapping[event.changedProperty][0]; - const value = - event.nodeProperties[topLevelKey as keyof ExportNodeProperties]; - yNode.set(topLevelKey, value); + this.yDoc.transact(() => { + const topLevelKey = NodePropertyMapping[event.changedProperty][0]; + const value = + event.nodeProperties[topLevelKey as keyof ExportNodeProperties]; + yNode.set(topLevelKey, value); + }, 'local'); } private writeNodeRemoveFromYDoc(nodeId: string): void { @@ -1416,7 +1445,7 @@ export class MapSyncService implements OnDestroy { for (const id of descendantIds) { nodesMap.delete(id); } - }); + }, 'local'); } private writeNodesPasteToYDoc(nodes: ExportNodeProperties[]): void { @@ -1427,7 +1456,7 @@ export class MapSyncService implements OnDestroy { this.populateYMapFromNodeProps(yNode, node); nodesMap.set(node.id, yNode); } - }); + }, 'local'); } private writeImportToYDoc(): void { @@ -1437,7 +1466,7 @@ export class MapSyncService implements OnDestroy { this.yDoc.transact(() => { this.clearAndRepopulateNodes(nodesMap, sorted); - }); + }, 'import'); } private clearAndRepopulateNodes( @@ -1454,68 +1483,14 @@ export class MapSyncService implements OnDestroy { } } - private writeUndoRedoDiffToYDoc(diff: MapDiff): void { - if (!diff) return; - const nodesMap = this.yDoc.getMap('nodes') as Y.Map>; - - this.yDoc.transact(() => { - this.writeAddedNodesToYDoc(nodesMap, diff.added); - this.writeUpdatedNodesToYDoc(nodesMap, diff.updated); - this.writeDeletedNodesFromYDoc(nodesMap, diff.deleted); - }); - } - - private writeAddedNodesToYDoc( - nodesMap: Y.Map>, - added: SnapshotChanges - ): void { - if (!added) return; - for (const nodeId in added) { - const nodeProps = added[nodeId] as ExportNodeProperties; - const yNode = new Y.Map(); - this.populateYMapFromNodeProps(yNode, nodeProps); - nodesMap.set(nodeId, yNode); - } - } - - private writeUpdatedNodesToYDoc( - nodesMap: Y.Map>, - updated: SnapshotChanges - ): void { - if (!updated) return; - for (const nodeId in updated) { - const yNode = nodesMap.get(nodeId); - if (!yNode) continue; - this.applyPropertyUpdatesToYMap(yNode, updated[nodeId]); - } - } - - private applyPropertyUpdatesToYMap( - yNode: Y.Map, - updates: Partial - ): void { - if (!updates) return; - for (const key in updates) { - yNode.set(key, (updates as Record)[key]); - } - } - - private writeDeletedNodesFromYDoc( - nodesMap: Y.Map>, - deleted: SnapshotChanges - ): void { - if (!deleted) return; - for (const nodeId in deleted) { - nodesMap.delete(nodeId); - } - } - private writeMapOptionsToYDoc(options?: CachedMapOptions): void { if (!this.yDoc || !options) return; const optionsMap = this.yDoc.getMap('mapOptions'); - optionsMap.set('fontMaxSize', options.fontMaxSize); - optionsMap.set('fontMinSize', options.fontMinSize); - optionsMap.set('fontIncrement', options.fontIncrement); + this.yDoc.transact(() => { + optionsMap.set('fontMaxSize', options.fontMaxSize); + optionsMap.set('fontMinSize', options.fontMinSize); + optionsMap.set('fontIncrement', options.fontIncrement); + }, 'local'); } private async deleteMapViaHttp(adminId: string): Promise { @@ -1552,7 +1527,7 @@ export class MapSyncService implements OnDestroy { events: Y.YEvent>>>[], transaction: Y.Transaction ) => { - if (transaction.local) return; + if (transaction.local && transaction.origin !== this.yUndoManager) return; for (const event of events) { this.handleYjsNodeEvent(event, nodesMap); } @@ -1584,12 +1559,14 @@ export class MapSyncService implements OnDestroy { return; } + const adds: string[] = []; + mapEvent.keysChanged.forEach(key => { const change = mapEvent.changes.keys.get(key); if (!change) return; if (change.action === 'add') { - this.applyRemoteNodeAdd(nodesMap.get(key)); + adds.push(key); } else if (change.action === 'update') { this.applyRemoteNodeDelete(key); this.applyRemoteNodeAdd(nodesMap.get(key)); @@ -1597,6 +1574,15 @@ export class MapSyncService implements OnDestroy { this.applyRemoteNodeDelete(key); } }); + + if (adds.length > 0) { + const nodeProps = adds + .map(key => nodesMap.get(key)) + .filter((yNode): yNode is Y.Map => !!yNode) + .map(yNode => this.yMapToNodeProps(yNode)); + const sorted = sortParentFirst(nodeProps); + sorted.forEach(props => this.mmpService.addNodesFromServer([props])); + } } // Show toast to inform the user that a remote client imported a map @@ -1660,7 +1646,7 @@ export class MapSyncService implements OnDestroy { private setupYjsMapOptionsObserver(): void { const optionsMap = this.yDoc.getMap('mapOptions'); this.yjsOptionsObserver = (_: unknown, transaction: Y.Transaction) => { - if (transaction.local) return; + if (transaction.local && transaction.origin !== this.yUndoManager) return; this.applyRemoteMapOptions(); }; optionsMap.observe(this.yjsOptionsObserver); diff --git a/teammapper-frontend/src/app/core/services/map-sync/yjs-undo-manager.spec.ts b/teammapper-frontend/src/app/core/services/map-sync/yjs-undo-manager.spec.ts new file mode 100644 index 00000000..4d9d6480 --- /dev/null +++ b/teammapper-frontend/src/app/core/services/map-sync/yjs-undo-manager.spec.ts @@ -0,0 +1,379 @@ +import * as Y from 'yjs'; +import { + populateYMapFromNodeProps, + yMapToNodeProps, + sortParentFirst, +} from './yjs-utils'; +import { ExportNodeProperties } from '@mmp/map/types'; + +describe('YjsUndoManager', () => { + const ORIGIN_LOCAL = 'local'; + const ORIGIN_IMPORT = 'import'; + + function createMockNode( + overrides?: Partial + ): ExportNodeProperties { + return { + id: 'mock-id', + name: 'Mock Node', + parent: 'root', + k: 1, + colors: { branch: '#000000' }, + font: { size: 14, style: 'normal', weight: 'normal' }, + locked: false, + hidden: false, + coordinates: undefined, + image: undefined, + link: undefined, + isRoot: false, + detached: false, + ...overrides, + }; + } + + function createYjsContext() { + const doc = new Y.Doc(); + const nodesMap = doc.getMap('nodes') as Y.Map>; + return { doc, nodesMap }; + } + + function createTrackedUndoManager(scope: Y.Map): Y.UndoManager { + return new Y.UndoManager(scope, { + trackedOrigins: new Set([ORIGIN_LOCAL]), + }); + } + + function addNodeToMap( + doc: Y.Doc, + nodesMap: Y.Map>, + nodeOverrides: Partial, + origin: string = ORIGIN_LOCAL + ): void { + doc.transact(() => { + const yNode = new Y.Map(); + populateYMapFromNodeProps(yNode, createMockNode(nodeOverrides)); + nodesMap.set(nodeOverrides.id!, yNode); + }, origin); + } + + function addRootNode(doc: Y.Doc, nodesMap: Y.Map>): void { + addNodeToMap( + doc, + nodesMap, + { + id: 'root', + parent: undefined, + isRoot: true, + coordinates: { x: 0, y: 0 }, + }, + ORIGIN_IMPORT + ); + } + + // ─── Origin filtering ──────────────────────────────────────── + + describe('origin filtering', () => { + let doc: Y.Doc; + let nodesMap: Y.Map>; + let undoManager: Y.UndoManager; + + beforeEach(() => { + ({ doc, nodesMap } = createYjsContext()); + undoManager = createTrackedUndoManager(nodesMap); + }); + + afterEach(() => { + undoManager.destroy(); + doc.destroy(); + }); + + it('captures local-origin transactions', () => { + addNodeToMap(doc, nodesMap, { id: 'n1', name: 'Test' }); + + expect(undoManager.undoStack.length).toBe(1); + }); + + it('ignores import-origin transactions', () => { + addNodeToMap(doc, nodesMap, { id: 'root', isRoot: true }, ORIGIN_IMPORT); + + expect(undoManager.undoStack.length).toBe(0); + }); + }); + + // ─── Lifecycle ─────────────────────────────────────────────── + + describe('lifecycle', () => { + it('does not capture operations performed before creation', () => { + const { doc, nodesMap } = createYjsContext(); + addNodeToMap(doc, nodesMap, { id: 'root', isRoot: true }); + + const undoManager = createTrackedUndoManager(nodesMap); + + expect(undoManager.undoStack.length).toBe(0); + undoManager.destroy(); + doc.destroy(); + }); + }); + + // ─── Echo prevention ───────────────────────────────────────── + + describe('echo prevention', () => { + let doc: Y.Doc; + let nodesMap: Y.Map>; + let undoManager: Y.UndoManager; + + beforeEach(() => { + ({ doc, nodesMap } = createYjsContext()); + }); + + afterEach(() => { + undoManager?.destroy(); + doc.destroy(); + }); + + function observeWithFilter(um: Y.UndoManager): string[] { + const calls: string[] = []; + nodesMap.observeDeep((_events, transaction) => { + if (transaction.local && transaction.origin !== um) { + calls.push('skipped'); + return; + } + calls.push('applied'); + }); + return calls; + } + + it('filters local edits in observer', () => { + undoManager = createTrackedUndoManager(nodesMap); + const calls = observeWithFilter(undoManager); + + addNodeToMap(doc, nodesMap, { id: 'n1' }); + + expect(calls).toEqual(['skipped']); + }); + + it('passes through UndoManager-originated edits', () => { + addNodeToMap(doc, nodesMap, { id: 'n1' }); + undoManager = createTrackedUndoManager(nodesMap); + addNodeToMap(doc, nodesMap, { id: 'n2' }); + + const calls = observeWithFilter(undoManager); + undoManager.undo(); + + expect(calls).toEqual(['applied']); + }); + + it('passes through remote edits', () => { + const remoteDoc = new Y.Doc(); + const remoteNodesMap = remoteDoc.getMap('nodes') as Y.Map>; + + undoManager = createTrackedUndoManager(nodesMap); + const calls = observeWithFilter(undoManager); + + remoteDoc.transact(() => { + const yNode = new Y.Map(); + populateYMapFromNodeProps(yNode, createMockNode({ id: 'r1' })); + remoteNodesMap.set('r1', yNode); + }); + + Y.applyUpdate(doc, Y.encodeStateAsUpdate(remoteDoc)); + + expect(calls).toEqual(['applied']); + remoteDoc.destroy(); + }); + }); + + // ─── canUndo/canRedo reactive state ────────────────────────── + + describe('canUndo/canRedo reactive state', () => { + let doc: Y.Doc; + let nodesMap: Y.Map>; + let undoManager: Y.UndoManager; + let canUndo: boolean; + let canRedo: boolean; + + beforeEach(() => { + ({ doc, nodesMap } = createYjsContext()); + undoManager = createTrackedUndoManager(nodesMap); + canUndo = false; + canRedo = false; + + const updateState = () => { + canUndo = undoManager.undoStack.length > 0; + canRedo = undoManager.redoStack.length > 0; + }; + undoManager.on('stack-item-added', updateState); + undoManager.on('stack-item-popped', updateState); + }); + + afterEach(() => { + undoManager.destroy(); + doc.destroy(); + }); + + it('enables canUndo after edit', () => { + addNodeToMap(doc, nodesMap, { id: 'n1' }); + + expect({ canUndo, canRedo }).toEqual({ + canUndo: true, + canRedo: false, + }); + }); + + it('enables canRedo after undo', () => { + addNodeToMap(doc, nodesMap, { id: 'n1' }); + undoManager.undo(); + + expect({ canUndo, canRedo }).toEqual({ + canUndo: false, + canRedo: true, + }); + }); + + it('clears redo stack on new edit after undo', () => { + addNodeToMap(doc, nodesMap, { id: 'n1' }); + undoManager.undo(); + addNodeToMap(doc, nodesMap, { id: 'n2' }); + + expect({ canUndo, canRedo }).toEqual({ + canUndo: true, + canRedo: false, + }); + }); + }); + + // ─── Undo/redo operations ──────────────────────────────────── + + describe('undo/redo operations', () => { + let doc: Y.Doc; + let nodesMap: Y.Map>; + let undoManager: Y.UndoManager; + + beforeEach(() => { + ({ doc, nodesMap } = createYjsContext()); + addRootNode(doc, nodesMap); + undoManager = createTrackedUndoManager(nodesMap); + }); + + afterEach(() => { + undoManager.destroy(); + doc.destroy(); + }); + + it('undo removes a created node', () => { + addNodeToMap(doc, nodesMap, { + id: 'n1', + name: 'Test', + coordinates: { x: 200, y: -120 }, + }); + + undoManager.undo(); + + expect(nodesMap.has('n1')).toBe(false); + }); + + it('redo restores an undone node with its properties', () => { + const coords = { x: 200, y: -120 }; + addNodeToMap(doc, nodesMap, { + id: 'n1', + name: 'Test', + coordinates: coords, + }); + undoManager.undo(); + + undoManager.redo(); + + expect(yMapToNodeProps(nodesMap.get('n1')!)).toEqual( + expect.objectContaining({ + id: 'n1', + name: 'Test', + coordinates: coords, + }) + ); + }); + + it('undo restores a deleted node with its coordinates', () => { + const coords = { x: 200, y: -120 }; + addNodeToMap(doc, nodesMap, { + id: 'n1', + parent: 'root', + coordinates: coords, + }); + undoManager.stopCapturing(); + doc.transact(() => nodesMap.delete('n1'), ORIGIN_LOCAL); + + undoManager.undo(); + + expect(yMapToNodeProps(nodesMap.get('n1')!)).toEqual( + expect.objectContaining({ id: 'n1', coordinates: coords }) + ); + }); + + it('batches subtree deletion into a single stack item', () => { + addNodeToMap(doc, nodesMap, { + id: 'A', + parent: 'root', + coordinates: { x: 200, y: -120 }, + }); + addNodeToMap(doc, nodesMap, { + id: 'B', + parent: 'A', + coordinates: { x: 400, y: -80 }, + }); + undoManager.stopCapturing(); + const stackBefore = undoManager.undoStack.length; + + doc.transact(() => { + nodesMap.delete('A'); + nodesMap.delete('B'); + }, ORIGIN_LOCAL); + + expect(undoManager.undoStack.length).toBe(stackBefore + 1); + }); + + it('undo restores a deleted subtree with coordinates', () => { + const coordsA = { x: 200, y: -120 }; + const coordsB = { x: 400, y: -80 }; + addNodeToMap(doc, nodesMap, { + id: 'A', + parent: 'root', + coordinates: coordsA, + }); + addNodeToMap(doc, nodesMap, { + id: 'B', + parent: 'A', + coordinates: coordsB, + }); + undoManager.stopCapturing(); + doc.transact(() => { + nodesMap.delete('A'); + nodesMap.delete('B'); + }, ORIGIN_LOCAL); + + undoManager.undo(); + + expect({ + size: nodesMap.size, + A: yMapToNodeProps(nodesMap.get('A')!).coordinates, + B: yMapToNodeProps(nodesMap.get('B')!).coordinates, + }).toEqual({ size: 3, A: coordsA, B: coordsB }); + }); + }); + + // ─── sortParentFirst ───────────────────────────────────────── + + describe('sortParentFirst', () => { + it('sorts deep hierarchy in parent-first order', () => { + const nodes = [ + createMockNode({ id: 'gc', parent: 'child' }), + createMockNode({ id: 'child', parent: 'parent' }), + createMockNode({ id: 'root', parent: undefined, isRoot: true }), + createMockNode({ id: 'parent', parent: 'root' }), + ]; + + const ids = sortParentFirst(nodes).map(n => n.id); + + expect(ids).toEqual(['root', 'parent', 'child', 'gc']); + }); + }); +}); diff --git a/teammapper-frontend/src/app/core/services/mmp/mmp.service.ts b/teammapper-frontend/src/app/core/services/mmp/mmp.service.ts index aa152f26..ecc1419b 100644 --- a/teammapper-frontend/src/app/core/services/mmp/mmp.service.ts +++ b/teammapper-frontend/src/app/core/services/mmp/mmp.service.ts @@ -4,7 +4,7 @@ import { SettingsService } from '../settings/settings.service'; import { ToastrService } from 'ngx-toastr'; import { UtilsService } from '../utils/utils.service'; import { jsPDF } from 'jspdf'; -import { first } from 'rxjs/operators'; +import { filter } from 'rxjs/operators'; import * as mmp from '@mmp/index'; import MmpMap from '@mmp/map/map'; import DOMPurify from 'dompurify'; @@ -47,8 +47,8 @@ export class MmpService implements OnDestroy { this.settingsSubscription = settingsService .getEditModeObservable() - .pipe(first((val: boolean | null) => val !== null)) - .subscribe((result: boolean | null) => { + .pipe(filter((val: boolean | null): val is boolean => val !== null)) + .subscribe((result: boolean) => { if (!this.currentMap) return; this.currentMap.options.update('drag', result); diff --git a/teammapper-frontend/src/app/modules/application/components/toolbar/toolbar.component.html b/teammapper-frontend/src/app/modules/application/components/toolbar/toolbar.component.html index 758853e9..30333307 100644 --- a/teammapper-frontend/src/app/modules/application/components/toolbar/toolbar.component.html +++ b/teammapper-frontend/src/app/modules/application/components/toolbar/toolbar.component.html @@ -175,9 +175,9 @@