diff --git a/.env.default b/.env.default index 3004140ab..d09d02757 100644 --- a/.env.default +++ b/.env.default @@ -42,6 +42,7 @@ POSTGRES_STATEMENT_TIMEOUT=100000 DELETE_AFTER_DAYS=30 YJS_ENABLED=true +AI_ENABLED=false DEV_BUILD_CONTEXT= diff --git a/README.md b/README.md index 32ccae7bd..eec703a2e 100644 --- a/README.md +++ b/README.md @@ -49,6 +49,8 @@ services: POSTGRES_QUERY_TIMEOUT: 100000 POSTGRES_STATEMENT_TIMEOUT: 100000 DELETE_AFTER_DAYS: 30 + YJS_ENABLED: true + AI_ENABLED: false ports: - 80:3000 depends_on: @@ -233,8 +235,19 @@ The settings are conceptually divided into: The settings configuration includes the following feature flags: -- `featureFlagPictograms`: Disables/Enables the pictogram feature (default: disabled). Note: You have to set this flag before build time! -- `featureFlagAI`: Disables/Enables AI functionality like generating mindmaps with AI +- `pictograms`: Disables/Enables the pictogram feature (default: disabled). Note: You have to set this flag before build time in the JSON config! +- `ai`: Disables/Enables AI functionality like generating mindmaps with AI. Can be controlled via the `AI_ENABLED` environment variable (default: `false`). +- `yjs`: Disables/Enables the Yjs-based real-time collaboration. Can be controlled via the `YJS_ENABLED` environment variable (default: `false`). + +#### Environment variable overrides + +The following environment variables override the feature flags from the JSON config: + +| Variable | Description | Default | +| --- | --- | --- | +| `AI_ENABLED` | Enable AI features (mindmap generation) | `false` | +| `YJS_ENABLED` | Enable Yjs-based real-time collaboration | `false` | +| `DELETE_AFTER_DAYS` | Number of days before mindmaps are deleted | `30` | ### Further details diff --git a/openspec/changes/yjs-auth-read-write-access/.openspec.yaml b/openspec/changes/yjs-auth-read-write-access/.openspec.yaml new file mode 100644 index 000000000..e3dce8f06 --- /dev/null +++ b/openspec/changes/yjs-auth-read-write-access/.openspec.yaml @@ -0,0 +1,2 @@ +schema: spec-driven +created: 2026-02-18 diff --git a/openspec/changes/yjs-auth-read-write-access/design.md b/openspec/changes/yjs-auth-read-write-access/design.md new file mode 100644 index 000000000..9d8712d80 --- /dev/null +++ b/openspec/changes/yjs-auth-read-write-access/design.md @@ -0,0 +1,101 @@ +## Context + +When a client connects to a TeamMapper map, it needs to know whether it has write access (correct modification secret) or is read-only. The current implementation sends this as a custom WebSocket binary message (type 4) before the Yjs sync messages. The client must hack y-websocket's `messageHandlers` array to prevent console errors and attach a raw WebSocket listener to parse the message. + +The `GET /api/maps/:id` HTTP call already happens before the WebSocket connection is established. The modification secret is already available in `MapSyncService.modificationSecret` before `fetchMapFromServer` is called (set in `prepareExistingMap`). This makes the HTTP response the natural place to communicate write-access status. + +Server-side enforcement is completely independent: `processReadOnlySyncMessage` in the Yjs gateway drops SyncStep2/Update messages from read-only clients regardless of how the client learns its permission level. This is the real security boundary and is unchanged. + +## Goals / Non-Goals + +**Goals:** +- Eliminate custom WebSocket message type 4 +- Remove y-websocket `messageHandlers` mutation +- Remove raw WebSocket `addEventListener` hack +- Communicate write-access via existing HTTP endpoint +- Keep server-side enforcement unchanged + +**Non-Goals:** +- Changing how the secret is stored or transmitted to the WebSocket (stays as query param) +- Implementing timing-safe comparison (low risk, deferred) +- Modifying the `HttpService` class (secret appended to URL string directly) + +## Decisions + +### 1. Secret passed as query parameter on HTTP GET + +**Decision:** Append `?secret=` to the `GET /api/maps/:id` URL when a modification secret is available. Use `encodeURIComponent()` for safety. + +**Rationale:** The secret is already passed as a query parameter in the WebSocket URL (`params: { secret: ... }`). Using the same pattern for the HTTP call is consistent. The `HttpService.get()` method only accepts `(apiUrl, endpoint)` — no headers/options support — so appending to the URL string is the simplest approach without modifying the service. + +**Alternative considered:** Adding headers support to `HttpService`. Rejected — over-engineering for this use case, and inconsistent with the WebSocket path which also uses query params. + +### 2. `writable` computed in the controller, not the service + +**Decision:** The `maps.controller.ts` `findOne` endpoint accepts the `@Query('secret')` parameter, calls `mapsService.findMap()` to get the `modificationSecret`, and uses `checkWriteAccess()` to compute `writable`. The result is spread into the response: `{ ...map, writable }`. + +**Rationale:** The `MapsService.exportMapToClient()` method is a generic export function used by multiple endpoints (create, duplicate, etc.). Adding secret-awareness to it would conflate concerns. The controller is the right place to handle request-specific parameters. + +**Trade-off:** `findMap()` is called separately from `exportMapToClient()` (which calls it internally via `updateLastAccessed`). This means two DB lookups for the same map. Acceptable because it's a simple PK lookup and avoids modifying the service interface. + +### 3. `writable` defaults to `true` when absent + +**Decision:** Frontend treats `serverMap.writable !== false` as writable. This means if the field is `undefined` (e.g., from a cached response or older server), the client defaults to writable and relies on server-side enforcement. + +**Rationale:** Fail-open on the client is safe because the server enforces read-only at the WebSocket level. A client that thinks it's writable but isn't will simply have its writes silently dropped. This is better than fail-closed (defaulting to read-only), which would lock out users unnecessarily. + +### 4. Removal scope: all custom message type 4 code + +**Decision:** Remove all code related to `MESSAGE_WRITE_ACCESS` (type 4) from both frontend and backend: + +**Backend removals:** +- `yjsProtocol.ts`: `MESSAGE_WRITE_ACCESS` constant, `encodeWriteAccessMessage()` function +- `yjs-gateway.service.ts`: `this.send(ws, encodeWriteAccessMessage(writable))` line in `setupSync()` + +**Frontend removals:** +- `yjs-utils.ts`: `MESSAGE_WRITE_ACCESS` constant, `parseWriteAccessBytes()` function +- `map-sync.service.ts`: `MESSAGE_WRITE_ACCESS` constant (line 64), `setupYjsWriteAccessListener()`, `attachWriteAccessListener()`, `parseWriteAccessMessage()`, and the call site `this.setupYjsWriteAccessListener()` in `setupYjsConnection` + +**Kept:** +- `checkWriteAccess()` in `yjsProtocol.ts` — reused by both the gateway (server-side enforcement) and the controller (HTTP response) +- `processReadOnlySyncMessage()` — server-side enforcement unchanged +- WebSocket `secret` query param — still used for server-side write enforcement + +## Data Flow + +``` +BEFORE (custom WebSocket message): +┌────────┐ GET /api/maps/:id ┌────────┐ +│ Client │─────────────────────▶│ Server │ (no write-access info) +│ │◀─────────────────────│ │ +│ │ │ │ +│ │ WS connect │ │ +│ │─────────────────────▶│ │ +│ │◀── msg type 4 ───── │ │ encodeWriteAccessMessage(writable) +│ │◀── SyncStep1 ────── │ │ +│ │◀── SyncStep2 ────── │ │ +│ │ messageHandlers[4] │ │ (client hacks y-websocket) +│ │ ws.addEventListener │ │ (client attaches raw listener) +└────────┘ └────────┘ + +AFTER (HTTP-based): +┌────────┐ GET /api/maps/:id?secret=... ┌────────┐ +│ Client │────────────────────────────────▶│ Server │ +│ │◀── { ...map, writable } ───────│ │ checkWriteAccess() +│ │ │ │ +│ │ WS connect │ │ +│ │────────────────────────────────▶│ │ +│ │◀── SyncStep1 ───────────────── │ │ (no custom message) +│ │◀── SyncStep2 ───────────────── │ │ +│ │ (no messageHandlers hack) │ │ +│ │ (no raw WS listener) │ │ +└────────┘ └────────┘ +``` + +## Risks / Trade-offs + +**[Double DB lookup in findOne]** The controller calls both `exportMapToClient()` (which internally calls `findMap()`) and `findMap()` directly. This is two PK lookups for the same row. Mitigation: PK lookups are fast and cached by the DB. Can be optimized later by extracting the map entity from `exportMapToClient` if needed. + +**[Secret in query string]** The secret appears in URL query parameters, which may be logged by proxies or appear in browser history. Mitigation: Already the case for the WebSocket URL. Can be moved to a header in a future iteration. + +**[Race between HTTP and WebSocket]** If the map's `modificationSecret` changes between the HTTP call and WebSocket connection, the client could have stale permission info. Mitigation: The server enforces permissions on every WebSocket message via `processReadOnlySyncMessage`. The client's `writable` flag only affects UI state (edit mode), not actual security. diff --git a/openspec/changes/yjs-auth-read-write-access/proposal.md b/openspec/changes/yjs-auth-read-write-access/proposal.md new file mode 100644 index 000000000..1585717af --- /dev/null +++ b/openspec/changes/yjs-auth-read-write-access/proposal.md @@ -0,0 +1,47 @@ +## Why + +The current implementation uses a custom WebSocket message type (`MESSAGE_WRITE_ACCESS = 4`) to communicate write permissions from server to client. This causes three problems: + +1. **Direct mutation of y-websocket internals**: The client sets `wsProvider.messageHandlers[4]` as a no-op to suppress `console.error('Unable to compute message')` for the unrecognized type. +2. **Fragile raw WebSocket listener**: A second `ws.addEventListener('message', ...)` is attached to parse the custom binary message, duplicating protocol handling outside y-websocket's message pipeline. +3. **Not within standard y-websocket types**: Types 0-3 are defined by y-websocket (sync, awareness, auth, queryAwareness). Type 4 is custom and could conflict with future y-websocket versions. + +The client already makes an HTTP `GET /api/maps/:id` call before establishing the WebSocket connection. The modification secret is already stored in `MapSyncService` before this call. Moving write-access determination to the HTTP response eliminates the custom message type entirely. + +## What Changes + +- **Backend `GET /api/maps/:id`**: Accept an optional `?secret=` query parameter. Look up the map's `modificationSecret`, compute `writable` using the existing `checkWriteAccess()` utility, and return it in the response. +- **Backend Yjs gateway**: Remove the `encodeWriteAccessMessage(writable)` send from `setupSync()`. Server-side enforcement (`processReadOnlySyncMessage`) is unchanged. +- **Backend protocol utils**: Remove `MESSAGE_WRITE_ACCESS` constant and `encodeWriteAccessMessage()` function. Keep `checkWriteAccess()` (reused by both gateway and controller). +- **Frontend `fetchMapFromServer`**: Append `?secret=...` to the HTTP request URL when a modification secret is set. +- **Frontend `prepareExistingMap`**: Set `yjsWritable` from the HTTP response's `writable` field instead of waiting for a WebSocket message. +- **Frontend cleanup**: Remove `setupYjsWriteAccessListener()`, `attachWriteAccessListener()`, `parseWriteAccessMessage()`, the `MESSAGE_WRITE_ACCESS` constant, and `parseWriteAccessBytes()` from yjs-utils. + +## Non-goals + +- Changing the server-side enforcement mechanism (`processReadOnlySyncMessage` silently drops writes from read-only clients — this is unchanged) +- Moving the secret to an HTTP header (consistent with existing WebSocket URL pattern; can be improved later) +- Timing-safe secret comparison (low risk — the secret is a shareable edit link token, not a cryptographic credential) + +## Yjs Logic (Unchanged) + +- If the secret is provided and correct: allow all modifications (writable WebSocket sync) +- If no / wrong secret is provided: silently drop all write operations on the server side, but allow the normal connection for reading +- The HTTP call handles communicating the permission level to the client UI + +## Capabilities + +### New Capabilities +- `yjs-write-access`: HTTP-based write-access determination replacing custom WebSocket message type 4 + +## Impact + +- **Backend `maps.controller.ts`**: Add `@Query('secret')` parameter, compute and return `writable` +- **Backend `types.ts`**: Add `writable?: boolean` to `IMmpClientMap` +- **Backend `yjs-gateway.service.ts`**: Remove `encodeWriteAccessMessage` send +- **Backend `yjsProtocol.ts`**: Remove `MESSAGE_WRITE_ACCESS`, `encodeWriteAccessMessage` +- **Frontend `server-types.ts`**: Add `writable?: boolean` to `ServerMap` +- **Frontend `map-sync.service.ts`**: Update `fetchMapFromServer`, `prepareExistingMap`; remove 3 write-access listener methods + constant +- **Frontend `yjs-utils.ts`**: Remove `MESSAGE_WRITE_ACCESS`, `parseWriteAccessBytes` +- **No new dependencies** +- **No breaking changes**: Custom message type 4 is internal to this branch diff --git a/openspec/changes/yjs-auth-read-write-access/specs/yjs-write-access/spec.md b/openspec/changes/yjs-auth-read-write-access/specs/yjs-write-access/spec.md new file mode 100644 index 000000000..581318102 --- /dev/null +++ b/openspec/changes/yjs-auth-read-write-access/specs/yjs-write-access/spec.md @@ -0,0 +1,75 @@ +## ADDED Requirements + +### Requirement: HTTP-based write-access determination +The `GET /api/maps/:id` endpoint SHALL accept an optional `secret` query parameter. When provided, the server SHALL compare it against the map's `modificationSecret` using the existing `checkWriteAccess()` utility and return a `writable` boolean field in the response. + +#### Scenario: Map with no modification secret +- **WHEN** a client requests `GET /api/maps/:id` for a map that has no `modificationSecret` +- **THEN** the response SHALL include `writable: true` +- **AND** this SHALL be the case regardless of whether `?secret=` is provided + +#### Scenario: Map with correct secret +- **WHEN** a client requests `GET /api/maps/:id?secret=` +- **AND** the map has a `modificationSecret` that matches the provided secret +- **THEN** the response SHALL include `writable: true` + +#### Scenario: Map with wrong or missing secret +- **WHEN** a client requests `GET /api/maps/:id` without a `secret` parameter (or with an incorrect one) +- **AND** the map has a `modificationSecret` +- **THEN** the response SHALL include `writable: false` + +### Requirement: Frontend reads write-access from HTTP response +When the frontend fetches a map via `fetchMapFromServer`, it SHALL pass the stored `modificationSecret` as a `?secret=` query parameter (URL-encoded). The `prepareExistingMap` method SHALL set `yjsWritable` from the response's `writable` field before the WebSocket connection is established. + +#### Scenario: Secret passed in HTTP request +- **WHEN** `modificationSecret` is set in `MapSyncService` +- **AND** `fetchMapFromServer` is called +- **THEN** the HTTP request URL SHALL include `?secret=` + +#### Scenario: No secret omits query parameter +- **WHEN** `modificationSecret` is empty or null +- **AND** `fetchMapFromServer` is called +- **THEN** the HTTP request URL SHALL NOT include a `secret` query parameter + +#### Scenario: writable set from HTTP response +- **WHEN** `prepareExistingMap` receives the server response +- **THEN** `yjsWritable` SHALL be set to `serverMap.writable !== false` +- **AND** `yjsWritable` SHALL be set BEFORE the WebSocket connection is established + +#### Scenario: writable defaults to true when absent +- **WHEN** the server response does not include a `writable` field +- **THEN** `yjsWritable` SHALL default to `true` +- **AND** server-side enforcement SHALL still apply (client writes silently dropped if unauthorized) + +### Requirement: Removal of custom WebSocket message type 4 +The custom `MESSAGE_WRITE_ACCESS` (type 4) WebSocket message SHALL be removed from both frontend and backend. The server SHALL NOT send write-access messages over WebSocket. The client SHALL NOT register custom message handlers or raw WebSocket listeners for write-access. + +#### Scenario: Server does not send write-access message +- **WHEN** a WebSocket connection is established in `setupSync()` +- **THEN** the server SHALL NOT call `encodeWriteAccessMessage()` or send a type 4 message +- **AND** the server SHALL still send SyncStep1, SyncStep2, and awareness messages + +#### Scenario: Client does not hack messageHandlers +- **WHEN** the `WebsocketProvider` is created +- **THEN** the client SHALL NOT modify `wsProvider.messageHandlers[4]` +- **AND** the client SHALL NOT attach raw `ws.addEventListener('message', ...)` listeners for write-access parsing + +#### Scenario: Protocol utils cleaned up +- **WHEN** the codebase is updated +- **THEN** `MESSAGE_WRITE_ACCESS` and `encodeWriteAccessMessage()` SHALL NOT exist in `yjsProtocol.ts` +- **AND** `MESSAGE_WRITE_ACCESS` and `parseWriteAccessBytes()` SHALL NOT exist in `yjs-utils.ts` +- **AND** `checkWriteAccess()` SHALL remain in `yjsProtocol.ts` (used by gateway and controller) + +### Requirement: Server-side enforcement unchanged +The `processReadOnlySyncMessage` function in the Yjs gateway SHALL continue to silently drop SyncStep2 and Update messages from read-only clients. This is the security boundary and is NOT affected by the move from WebSocket to HTTP for permission signaling. + +#### Scenario: Read-only client writes are dropped +- **WHEN** a client connected without a valid secret sends a Y.Doc update +- **THEN** the server SHALL silently drop the update via `processReadOnlySyncMessage` +- **AND** no error SHALL be sent to the client (silent drop allows continued reading) + +## REMOVED Requirements + +### Requirement: WebSocket write-access message +**Reason**: Replaced by HTTP-based `writable` field in `GET /api/maps/:id` response. The custom WebSocket message type 4 (`MESSAGE_WRITE_ACCESS`) was a non-standard extension that required hacking y-websocket internals on the client side. +**Migration**: Write-access is now determined from the HTTP response before the WebSocket connection. Server-side enforcement is unchanged. diff --git a/openspec/changes/yjs-auth-read-write-access/tasks.md b/openspec/changes/yjs-auth-read-write-access/tasks.md new file mode 100644 index 000000000..e385cf879 --- /dev/null +++ b/openspec/changes/yjs-auth-read-write-access/tasks.md @@ -0,0 +1,31 @@ +## 1. Backend: Add `writable` to HTTP response + +- [x] 1.1 Add `writable?: boolean` to `IMmpClientMap` interface in `teammapper-backend/src/map/types.ts` +- [x] 1.2 Import `Query` from `@nestjs/common` and `checkWriteAccess` from `../utils/yjsProtocol` in `maps.controller.ts` +- [x] 1.3 Add `@Query('secret') secret?: string` parameter to `findOne` endpoint +- [x] 1.4 Call `mapsService.findMap(mapId)` to get the map's `modificationSecret`, compute `writable` via `checkWriteAccess()`, and return `{ ...map, writable }` +- [x] 1.5 Write unit tests: no secret on unprotected map returns `writable: true`, correct secret returns `writable: true`, wrong/missing secret returns `writable: false` + +## 2. Backend: Remove WebSocket write-access message + +- [x] 2.1 Remove `this.send(ws, encodeWriteAccessMessage(writable))` from `setupSync()` in `yjs-gateway.service.ts` +- [x] 2.2 Remove `encodeWriteAccessMessage` import from `yjs-gateway.service.ts` +- [x] 2.3 Remove `MESSAGE_WRITE_ACCESS` constant and `encodeWriteAccessMessage()` function from `yjsProtocol.ts` +- [x] 2.4 Remove or update the write-access message ordering test in `yjs-gateway.service.spec.ts` + +## 3. Frontend: Read `writable` from HTTP response + +- [x] 3.1 Add `writable?: boolean` to `ServerMap` interface in `server-types.ts` +- [x] 3.2 Update `fetchMapFromServer` to append `?secret=` when `modificationSecret` is set +- [x] 3.3 Set `this.yjsWritable = serverMap.writable !== false` in `prepareExistingMap` after fetching +- [x] 3.4 Write unit tests: `fetchMapFromServer` appends secret param when set, omits when empty; `prepareExistingMap` sets `yjsWritable` correctly for `true`, `false`, and `undefined` + +## 4. Frontend: Remove WebSocket write-access listener code + +- [x] 4.1 Remove `setupYjsWriteAccessListener()` method from `map-sync.service.ts` +- [x] 4.2 Remove `attachWriteAccessListener()` method from `map-sync.service.ts` +- [x] 4.3 Remove `parseWriteAccessMessage()` method from `map-sync.service.ts` +- [x] 4.4 Remove `this.setupYjsWriteAccessListener()` call from `setupYjsConnection` +- [x] 4.5 Remove `const MESSAGE_WRITE_ACCESS = 4` constant from `map-sync.service.ts` (line 64) +- [x] 4.6 Remove `MESSAGE_WRITE_ACCESS` constant and `parseWriteAccessBytes()` function from `yjs-utils.ts` +- [x] 4.7 Remove or update tests for `parseWriteAccessBytes` and `parseWriteAccessMessage` in `map-sync.service.spec.ts` diff --git a/openspec/changes/yjs-introduction/design.md b/openspec/changes/yjs-introduction/design.md index 133046eda..cc913a346 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 32e884a6b..6b43a2714 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 4108839b6..1bb1e04b0 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/.openspec.yaml b/openspec/changes/yjs-undo-redo-manager/.openspec.yaml new file mode 100644 index 000000000..c8d3976ae --- /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 000000000..eca3b2c28 --- /dev/null +++ b/openspec/changes/yjs-undo-redo-manager/design.md @@ -0,0 +1,154 @@ +## 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. + +### 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. + +**[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? + +## 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 new file mode 100644 index 000000000..463f0924a --- /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 + +- 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) + +## 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 000000000..7f651e2e8 --- /dev/null +++ b/openspec/changes/yjs-undo-redo-manager/specs/yjs-bridge/spec.md @@ -0,0 +1,88 @@ +## 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 + +### 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 +**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 000000000..05e624858 --- /dev/null +++ b/openspec/changes/yjs-undo-redo-manager/specs/yjs-undo/spec.md @@ -0,0 +1,143 @@ +## 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: 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. + +#### 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 000000000..f3ae96052 --- /dev/null +++ b/openspec/changes/yjs-undo-redo-manager/tasks.md @@ -0,0 +1,73 @@ +## 1. Add transaction origins to all Y.Doc write operations + +- [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 + +- [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 + +- [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 + +- [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 + +- [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 + +- [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 + +- [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/config.service.ts b/teammapper-backend/src/config.service.ts index e8472223f..4b28570a7 100644 --- a/teammapper-backend/src/config.service.ts +++ b/teammapper-backend/src/config.service.ts @@ -56,6 +56,11 @@ class ConfigService { return value?.toLowerCase() === 'true' } + public isAiEnabled(): boolean { + const value = this.getValue('AI_ENABLED', false) + return value?.toLowerCase() === 'true' + } + public isWsTrustProxy(): boolean { const value = this.getValue('WS_TRUST_PROXY', false) return value?.toLowerCase() === 'true' diff --git a/teammapper-backend/src/map/controllers/maps.controller.spec.ts b/teammapper-backend/src/map/controllers/maps.controller.spec.ts index 87c8440a8..79779df19 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() @@ -104,14 +94,16 @@ describe('MapsController', () => { const exportedMap: IMmpClientMap = createMmpClientMap({ id: mapId, }) + const mmpMap = createMmpMap({ modificationSecret: null }) jest .spyOn(mapsService, 'exportMapToClient') .mockResolvedValueOnce(exportedMap) + jest.spyOn(mapsService, 'findMap').mockResolvedValueOnce(mmpMap) const response = await mapsController.findOne(mapId) - expect(response).toEqual(exportedMap) + expect(response).toEqual({ ...exportedMap, writable: true }) }) it("should throw a NotFoundException if the map wasn't found", async () => { @@ -121,10 +113,70 @@ describe('MapsController', () => { .spyOn(mapsService, 'exportMapToClient') .mockRejectedValueOnce(new MalformedUUIDError('MalformedUUIDError')) - expect(mapsController.findOne(invalidMapId)).rejects.toThrow( + await expect(mapsController.findOne(invalidMapId)).rejects.toThrow( NotFoundException ) }) + + it('returns writable true when map has no modification secret', async () => { + const mapId = 'e7f66b65-ffd5-4387-b645-35f8e794c7e7' + const exportedMap: IMmpClientMap = createMmpClientMap({ id: mapId }) + const mmpMap = createMmpMap({ modificationSecret: null }) + + jest + .spyOn(mapsService, 'exportMapToClient') + .mockResolvedValueOnce(exportedMap) + jest.spyOn(mapsService, 'findMap').mockResolvedValueOnce(mmpMap) + + const response = await mapsController.findOne(mapId) + + expect(response).toEqual({ ...exportedMap, writable: true }) + }) + + it('returns writable true when correct secret is provided', async () => { + const mapId = 'e7f66b65-ffd5-4387-b645-35f8e794c7e7' + const exportedMap: IMmpClientMap = createMmpClientMap({ id: mapId }) + const mmpMap = createMmpMap({ modificationSecret: 'my-secret' }) + + jest + .spyOn(mapsService, 'exportMapToClient') + .mockResolvedValueOnce(exportedMap) + jest.spyOn(mapsService, 'findMap').mockResolvedValueOnce(mmpMap) + + const response = await mapsController.findOne(mapId, 'my-secret') + + expect(response).toEqual({ ...exportedMap, writable: true }) + }) + + it('returns writable false when wrong secret is provided', async () => { + const mapId = 'e7f66b65-ffd5-4387-b645-35f8e794c7e7' + const exportedMap: IMmpClientMap = createMmpClientMap({ id: mapId }) + const mmpMap = createMmpMap({ modificationSecret: 'my-secret' }) + + jest + .spyOn(mapsService, 'exportMapToClient') + .mockResolvedValueOnce(exportedMap) + jest.spyOn(mapsService, 'findMap').mockResolvedValueOnce(mmpMap) + + const response = await mapsController.findOne(mapId, 'wrong-secret') + + expect(response).toEqual({ ...exportedMap, writable: false }) + }) + + it('returns writable false when no secret is provided for protected map', async () => { + const mapId = 'e7f66b65-ffd5-4387-b645-35f8e794c7e7' + const exportedMap: IMmpClientMap = createMmpClientMap({ id: mapId }) + const mmpMap = createMmpMap({ modificationSecret: 'my-secret' }) + + jest + .spyOn(mapsService, 'exportMapToClient') + .mockResolvedValueOnce(exportedMap) + jest.spyOn(mapsService, 'findMap').mockResolvedValueOnce(mmpMap) + + const response = await mapsController.findOne(mapId) + + expect(response).toEqual({ ...exportedMap, writable: false }) + }) }) describe('findAll', () => { diff --git a/teammapper-backend/src/map/controllers/maps.controller.ts b/teammapper-backend/src/map/controllers/maps.controller.ts index d372cdf4c..3e811d200 100644 --- a/teammapper-backend/src/map/controllers/maps.controller.ts +++ b/teammapper-backend/src/map/controllers/maps.controller.ts @@ -7,9 +7,13 @@ import { NotFoundException, Param, Post, + Query, Logger, + Optional, + Inject, } from '@nestjs/common' import { MapsService } from '../services/maps.service' +import { checkWriteAccess } from '../utils/yjsProtocol' import { YjsDocManagerService } from '../services/yjs-doc-manager.service' import { YjsGateway } from './yjs-gateway.service' import { @@ -22,26 +26,37 @@ 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') - async findOne(@Param('id') mapId: string): Promise { + async findOne( + @Param('id') mapId: string, + @Query('secret') secret?: string + ): Promise { try { - // If we update lastAccessed first, we guarantee that the exportMapToClient returns a fresh map that includes an up-to-date lastAccessed field await this.mapsService.updateLastAccessed(mapId) const map = await this.mapsService.exportMapToClient(mapId) if (!map) throw new NotFoundException() - return map + const fullMap = await this.mapsService.findMap(mapId) + const writable = checkWriteAccess( + fullMap?.modificationSecret ?? null, + secret ?? null + ) + + return { ...map, writable } } catch (e) { if (e instanceof MalformedUUIDError || e instanceof EntityNotFoundError) { throw new NotFoundException() @@ -67,7 +82,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 c3c81dc00..8bb95207a 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,12 @@ import { WS_CLOSE_MAP_NOT_FOUND, WS_CLOSE_TRY_AGAIN, CONNECTION_SETUP_TIMEOUT_MS, - encodeSyncStep1Message, + MESSAGE_SYNC, 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 +90,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 +166,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 +180,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('does not send a write-access message over WebSocket', async () => { mapsService.findMap.mockResolvedValue(createMockMap()) const ws = createMockWs() @@ -199,13 +204,18 @@ 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)) + }) + + expect(messageTypes).not.toContain(4) + expect(messageTypes[0]).toBe(MESSAGE_SYNC) }) - }) - 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 +224,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 +237,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 +274,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 +340,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 +359,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 +369,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 +386,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 +458,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 703830ba3..1da87ef4d 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,9 +35,9 @@ import { checkWriteAccess, toUint8Array, encodeSyncStep1Message, + encodeSyncStep2Message, encodeSyncUpdateMessage, encodeAwarenessMessage, - encodeWriteAccessMessage, processReadOnlySyncMessage, parseAwarenessClientIds, } from '../utils/yjsProtocol' @@ -73,11 +72,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 +335,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. SyncStep1 — server state vector (triggers client's SyncStep2) + // 2. SyncStep2 — full doc state for immediate hydration + // 3. Awareness — existing cursors/presence + // + // Write-access is communicated via the HTTP GET /api/maps/:id response. 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 2f4d142f9..23dbc6509 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/types.ts b/teammapper-backend/src/map/types.ts index 529ee784f..5847dcb74 100644 --- a/teammapper-backend/src/map/types.ts +++ b/teammapper-backend/src/map/types.ts @@ -30,6 +30,7 @@ export interface IMmpClientMap { data: IMmpClientNode[] options: IMmpClientMapOptions createdAt: Date | null + writable?: boolean } export interface IMmpClientMapInfo { diff --git a/teammapper-backend/src/map/utils/yjsProtocol.ts b/teammapper-backend/src/map/utils/yjsProtocol.ts index eff8f8dca..87e8824e6 100644 --- a/teammapper-backend/src/map/utils/yjsProtocol.ts +++ b/teammapper-backend/src/map/utils/yjsProtocol.ts @@ -7,9 +7,6 @@ import * as decoding from 'lib0/decoding' // Message types matching y-websocket client protocol export const MESSAGE_SYNC = 0 export const MESSAGE_AWARENESS = 1 -// Custom message type for communicating write access to clients -// (types 2=auth and 3=queryAwareness are reserved by y-websocket) -export const MESSAGE_WRITE_ACCESS = 4 // WebSocket close codes (4000-4999 private-use range per RFC 6455) export const WS_CLOSE_MISSING_PARAM = 4000 @@ -51,6 +48,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() @@ -73,14 +80,6 @@ export const encodeAwarenessMessage = ( return encoding.toUint8Array(encoder) } -// Encodes a write-access message to inform the client of its permissions -export const encodeWriteAccessMessage = (writable: boolean): Uint8Array => { - const encoder = encoding.createEncoder() - encoding.writeVarUint(encoder, MESSAGE_WRITE_ACCESS) - encoding.writeVarUint(encoder, writable ? 1 : 0) - return encoding.toUint8Array(encoder) -} - // For read-only clients: process SyncStep1 (state request), drop writes export const processReadOnlySyncMessage = ( decoder: decoding.Decoder, diff --git a/teammapper-backend/src/settings/settings.service.spec.ts b/teammapper-backend/src/settings/settings.service.spec.ts new file mode 100644 index 000000000..adfe936d6 --- /dev/null +++ b/teammapper-backend/src/settings/settings.service.spec.ts @@ -0,0 +1,172 @@ +import { jest } from '@jest/globals' +import * as fs from 'fs' +import configService from '../config.service' +import { SettingsService } from './settings.service' +import { Settings } from './settings.types' + +jest.mock('fs') +jest.mock('../config.service') + +const mockedFs = fs as jest.Mocked + +const defaultSettings: Settings = { + systemSettings: { + info: { name: 'TeamMapper', version: '1.0.0' }, + urls: { + pictogramApiUrl: 'https://api.example.com', + pictogramStaticUrl: 'https://static.example.com', + }, + featureFlags: { pictograms: true, ai: false, yjs: false }, + }, + userSettings: { + general: { language: 'en' }, + mapOptions: { + centerOnResize: false, + autoBranchColors: true, + showLinktext: false, + fontMaxSize: 70, + fontMinSize: 15, + fontIncrement: 5, + defaultNode: { + name: '', + link: { href: '' }, + image: { src: '', size: 60 }, + colors: { name: '#666666', background: '#f5f5f5', branch: '#546e7a' }, + font: { size: 22, style: 'normal', weight: 'normal' }, + locked: true, + }, + rootNode: { + name: 'Root node', + link: { href: '' }, + image: { src: '', size: 70 }, + colors: { name: '#666666', background: '#f5f5f5' }, + font: { size: 26, style: 'normal', weight: 'normal' }, + }, + }, + }, +} + +describe('SettingsService', () => { + let service: SettingsService + + beforeEach(() => { + jest.clearAllMocks() + ;(configService.isYjsEnabled as jest.Mock).mockReturnValue(false) + ;(configService.isAiEnabled as jest.Mock).mockReturnValue(false) + mockedFs.readFileSync.mockReturnValue(JSON.stringify(defaultSettings)) + mockedFs.existsSync.mockReturnValue(false) + + service = new SettingsService() + }) + + describe('loading default settings', () => { + it('should return settings parsed from the default config file', () => { + const settings = service.getSettings() + + expect(settings.systemSettings.info).toEqual({ + name: 'TeamMapper', + version: '1.0.0', + }) + }) + + it('should include user settings from the default config file', () => { + const settings = service.getSettings() + + expect(settings.userSettings.general.language).toBe('en') + }) + }) + + describe('override settings', () => { + it('should deep merge override file into default settings', () => { + const overrideSettings = { + systemSettings: { + info: { version: '2.0.0' }, + }, + } + mockedFs.existsSync.mockReturnValue(true) + mockedFs.statSync.mockReturnValue({ size: 100 } as fs.Stats) + mockedFs.readFileSync + .mockReturnValueOnce(JSON.stringify(defaultSettings)) + .mockReturnValueOnce(JSON.stringify(overrideSettings)) + + const settings = service.getSettings() + + expect(settings.systemSettings.info).toEqual({ + name: 'TeamMapper', + version: '2.0.0', + }) + }) + + it('should skip override file when it does not exist', () => { + mockedFs.existsSync.mockReturnValue(false) + + const settings = service.getSettings() + + expect(settings.systemSettings.info.version).toBe('1.0.0') + }) + + it('should skip override file when it is empty', () => { + mockedFs.existsSync.mockReturnValue(true) + mockedFs.statSync.mockReturnValue({ size: 0 } as fs.Stats) + + const settings = service.getSettings() + + expect(settings.systemSettings.info.version).toBe('1.0.0') + }) + }) + + describe('feature flags', () => { + it('should set ai flag to true when AI_ENABLED is true', () => { + ;(configService.isAiEnabled as jest.Mock).mockReturnValue(true) + + const settings = service.getSettings() + + expect(settings.systemSettings.featureFlags.ai).toBe(true) + }) + + it('should set ai flag to false when AI_ENABLED is false', () => { + ;(configService.isAiEnabled as jest.Mock).mockReturnValue(false) + + const settings = service.getSettings() + + expect(settings.systemSettings.featureFlags.ai).toBe(false) + }) + + it('should set yjs flag to true when YJS_ENABLED is true', () => { + ;(configService.isYjsEnabled as jest.Mock).mockReturnValue(true) + + const settings = service.getSettings() + + expect(settings.systemSettings.featureFlags.yjs).toBe(true) + }) + + it('should set yjs flag to false when YJS_ENABLED is false', () => { + ;(configService.isYjsEnabled as jest.Mock).mockReturnValue(false) + + const settings = service.getSettings() + + expect(settings.systemSettings.featureFlags.yjs).toBe(false) + }) + + it('should override file-based feature flags with config service values', () => { + const settingsWithFlags = { + ...defaultSettings, + systemSettings: { + ...defaultSettings.systemSettings, + featureFlags: { pictograms: true, ai: true, yjs: true }, + }, + } + mockedFs.readFileSync.mockReturnValue(JSON.stringify(settingsWithFlags)) + ;(configService.isAiEnabled as jest.Mock).mockReturnValue(false) + ;(configService.isYjsEnabled as jest.Mock).mockReturnValue(false) + + const settings = service.getSettings() + + expect(settings.systemSettings.featureFlags).toEqual({ + pictograms: true, + ai: false, + yjs: false, + }) + }) + }) +}) diff --git a/teammapper-backend/src/settings/settings.service.ts b/teammapper-backend/src/settings/settings.service.ts index 5aaa0bafb..e5f12d3d2 100644 --- a/teammapper-backend/src/settings/settings.service.ts +++ b/teammapper-backend/src/settings/settings.service.ts @@ -39,6 +39,7 @@ export class SettingsService { const settings = deepmerge(defaultSettings, overrideSettings) as Settings settings.systemSettings.featureFlags.yjs = configService.isYjsEnabled() + settings.systemSettings.featureFlags.ai = configService.isAiEnabled() return settings } } diff --git a/teammapper-backend/test/jest-setup.ts b/teammapper-backend/test/jest-setup.ts index 644679e14..c36664bd2 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 905d4e0f1..c7295cd67 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-context.ts b/teammapper-frontend/src/app/core/services/map-sync/map-sync-context.ts new file mode 100644 index 000000000..d28fc7468 --- /dev/null +++ b/teammapper-frontend/src/app/core/services/map-sync/map-sync-context.ts @@ -0,0 +1,24 @@ +import { ExportNodeProperties } from '@mmp/map/types'; +import { CachedMapEntry } from '../../../shared/models/cached-map.model'; +import { ClientColorMapping } from './yjs-utils'; + +export const DEFAULT_COLOR = '#000000'; +export const DEFAULT_SELF_COLOR = '#c0c0c0'; + +export type ConnectionStatus = 'connected' | 'disconnected' | null; + +export interface MapSyncContext { + getAttachedMap(): CachedMapEntry; + getModificationSecret(): string; + getColorMapping(): ClientColorMapping; + getClientColor(): string; + colorForNode(nodeId: string): string; + setConnectionStatus(status: ConnectionStatus): void; + setColorMapping(mapping: ClientColorMapping): void; + setAttachedNode(node: ExportNodeProperties | null): void; + setClientColor(color: string): void; + setCanUndo(v: boolean): void; + setCanRedo(v: boolean): void; + updateAttachedMap(): Promise; + emitClientList(): void; +} diff --git a/teammapper-frontend/src/app/core/services/map-sync/map-sync-error-handler.spec.ts b/teammapper-frontend/src/app/core/services/map-sync/map-sync-error-handler.spec.ts new file mode 100644 index 000000000..f7c959f4f --- /dev/null +++ b/teammapper-frontend/src/app/core/services/map-sync/map-sync-error-handler.spec.ts @@ -0,0 +1,210 @@ +import { MmpService } from '../mmp/mmp.service'; +import { UtilsService } from '../utils/utils.service'; +import { ToastService } from '../toast/toast.service'; +import { DialogService } from '../dialog/dialog.service'; +import { + ValidationErrorResponse, + CriticalErrorResponse, + SuccessResponse, + OperationResponse, +} from './server-types'; +import { ExportNodeProperties } from '@mmp/map/types'; +import { createMockUtilsService } from '../../../../test/mocks/utils-service.mock'; +import { MapSyncErrorHandler } from './map-sync-error-handler'; + +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, + }; +} + +describe('MapSyncErrorHandler', () => { + let handler: MapSyncErrorHandler; + let mmpService: jest.Mocked; + let utilsService: jest.Mocked; + let toastService: jest.Mocked; + let dialogService: jest.Mocked; + + const mockNode = createMockNode({ id: 'node-1', name: 'Test Node' }); + const mockMapSnapshot: ExportNodeProperties[] = [mockNode]; + + const mockServerMap = { + uuid: 'test-uuid', + lastModified: new Date().toISOString(), + deletedAt: new Date(Date.now() + 86400000).toISOString(), + deleteAfterDays: 30, + data: mockMapSnapshot, + options: { fontMaxSize: 18, fontMinSize: 10, fontIncrement: 2 }, + createdAt: new Date().toISOString(), + }; + + beforeEach(() => { + mmpService = { + new: jest.fn(), + } as unknown as jest.Mocked; + + utilsService = createMockUtilsService(); + + toastService = { + showValidationCorrection: jest.fn(), + } as unknown as jest.Mocked; + + dialogService = { + openCriticalErrorDialog: jest.fn(), + } as unknown as jest.Mocked; + + handler = new MapSyncErrorHandler( + mmpService, + utilsService, + toastService, + dialogService + ); + }); + + it('success response triggers no side effects', async () => { + const response: SuccessResponse = { + success: true, + data: [mockNode], + }; + + await handler.handleOperationResponse(response, 'add node'); + + expect({ + mapReloaded: mmpService.new.mock.calls.length, + toastShown: toastService.showValidationCorrection.mock.calls.length, + dialogOpened: dialogService.openCriticalErrorDialog.mock.calls.length, + }).toEqual({ mapReloaded: 0, toastShown: 0, dialogOpened: 0 }); + }); + + it('error with fullMapState reloads map', async () => { + const response: ValidationErrorResponse = { + success: false, + errorType: 'validation', + code: 'INVALID_PARENT', + message: 'Invalid parent', + fullMapState: mockServerMap, + }; + + await handler.handleOperationResponse(response, 'add node'); + + expect(mmpService.new).toHaveBeenCalledWith(mockMapSnapshot, false); + }); + + it('error with fullMapState shows toast', async () => { + const response: CriticalErrorResponse = { + success: false, + errorType: 'critical', + code: 'SERVER_ERROR', + message: 'Server error', + fullMapState: mockServerMap, + }; + + await handler.handleOperationResponse(response, 'add node'); + + expect(toastService.showValidationCorrection).toHaveBeenCalledWith( + 'add node', + 'Operation failed - map reloaded from server' + ); + }); + + it('error without fullMapState shows critical dialog', async () => { + const response: CriticalErrorResponse = { + success: false, + errorType: 'critical', + code: 'SERVER_ERROR', + message: 'Server error', + }; + + await handler.handleOperationResponse(response, 'add node'); + + expect(dialogService.openCriticalErrorDialog).toHaveBeenCalledWith({ + code: 'SERVER_ERROR', + message: expect.stringContaining('server encountered an error'), + }); + }); + + it('malformed response shows critical dialog', async () => { + const response = { + invalid: 'response', + } as unknown as OperationResponse; + + await handler.handleOperationResponse(response, 'add node'); + + expect(dialogService.openCriticalErrorDialog).toHaveBeenCalledWith({ + code: 'MALFORMED_RESPONSE', + message: expect.stringContaining('invalid response'), + }); + }); + + it('malformed response with translation failure uses fallback', async () => { + utilsService.translate.mockImplementation(async () => { + throw new Error('Translation failed'); + }); + + const response = { + invalid: 'response', + } as unknown as OperationResponse; + + await handler.handleOperationResponse(response, 'add node'); + + expect(dialogService.openCriticalErrorDialog).toHaveBeenCalledWith({ + code: 'MALFORMED_RESPONSE', + message: 'Invalid server response. Please try again.', + }); + }); + + it('error with malformed fullMapState shows critical dialog', async () => { + const response: ValidationErrorResponse = { + success: false, + errorType: 'validation', + code: 'INVALID_PARENT', + message: 'Invalid parent', + fullMapState: { + uuid: 'test-uuid', + data: [], + } as unknown as typeof mockServerMap, + }; + + await handler.handleOperationResponse(response, 'add node'); + + expect(dialogService.openCriticalErrorDialog).toHaveBeenCalledWith({ + code: 'MALFORMED_RESPONSE', + message: expect.stringContaining('invalid response'), + }); + }); + + it('error without fullMapState with translation failure uses fallback', async () => { + utilsService.translate.mockImplementation(async () => { + throw new Error('Translation failed'); + }); + + const response: CriticalErrorResponse = { + success: false, + errorType: 'critical', + code: 'SERVER_ERROR', + message: 'Server error', + }; + + await handler.handleOperationResponse(response, 'add node'); + + expect(dialogService.openCriticalErrorDialog).toHaveBeenCalledWith({ + code: 'SERVER_ERROR', + message: 'An error occurred. Please try again.', + }); + }); +}); diff --git a/teammapper-frontend/src/app/core/services/map-sync/map-sync-error-handler.ts b/teammapper-frontend/src/app/core/services/map-sync/map-sync-error-handler.ts new file mode 100644 index 000000000..ba8145b0f --- /dev/null +++ b/teammapper-frontend/src/app/core/services/map-sync/map-sync-error-handler.ts @@ -0,0 +1,155 @@ +import { MmpService } from '../mmp/mmp.service'; +import { UtilsService } from '../utils/utils.service'; +import { ToastService } from '../toast/toast.service'; +import { DialogService } from '../dialog/dialog.service'; +import { + OperationResponse, + ValidationErrorResponse, + CriticalErrorResponse, + ServerMap, +} from './server-types'; + +// Validate ServerMap structure at runtime +export function isValidServerMap(map: unknown): map is ServerMap { + if (!map || typeof map !== 'object') return false; + + const serverMap = map as ServerMap; + + return ( + typeof serverMap.uuid === 'string' && + serverMap.uuid.length > 0 && + Array.isArray(serverMap.data) && + serverMap.data.length > 0 && + typeof serverMap.lastModified === 'string' && + typeof serverMap.createdAt === 'string' && + typeof serverMap.deletedAt === 'string' && + typeof serverMap.deleteAfterDays === 'number' && + typeof serverMap.options === 'object' + ); +} + +// Type guard to validate error response structure at runtime +export function isValidErrorResponse( + response: OperationResponse +): response is ValidationErrorResponse | CriticalErrorResponse { + if (response.success !== false) return false; + + const errorResponse = response as + | ValidationErrorResponse + | CriticalErrorResponse; + + const isBasicStructureValid = + typeof errorResponse.errorType === 'string' && + (errorResponse.errorType === 'validation' || + errorResponse.errorType === 'critical') && + typeof errorResponse.code === 'string' && + errorResponse.code.trim() !== '' && + typeof errorResponse.message === 'string'; + + if (!isBasicStructureValid) return false; + + if (errorResponse.fullMapState) { + return isValidServerMap(errorResponse.fullMapState); + } + + return true; +} + +export class MapSyncErrorHandler { + constructor( + private mmpService: MmpService, + private utilsService: UtilsService, + private toastService: ToastService, + private dialogService: DialogService + ) {} + + // Simplified handler for all operation responses + async handleOperationResponse( + response: OperationResponse, + operationName: string + ): Promise { + if (response.success) { + return; + } + + if (!isValidErrorResponse(response)) { + await this.showMalformedResponseError(); + return; + } + + if (response.fullMapState) { + await this.handleRecoverableError(response, operationName); + } else { + await this.handleCriticalError(response); + } + } + + private async showMalformedResponseError(): Promise { + let message: string; + try { + message = await this.utilsService.translate( + 'TOASTS.ERRORS.MALFORMED_RESPONSE' + ); + } catch { + message = 'Invalid server response. Please try again.'; + } + this.dialogService.openCriticalErrorDialog({ + code: 'MALFORMED_RESPONSE', + message, + }); + } + + private async handleRecoverableError( + response: ValidationErrorResponse | CriticalErrorResponse, + operationName: string + ): Promise { + this.mmpService.new(response.fullMapState.data, false); + + let message: string; + try { + message = await this.utilsService.translate( + 'TOASTS.ERRORS.OPERATION_FAILED_MAP_RELOADED' + ); + } catch { + message = 'Operation failed - map reloaded'; + } + this.toastService.showValidationCorrection(operationName, message); + } + + private async handleCriticalError( + response: ValidationErrorResponse | CriticalErrorResponse + ): Promise { + const userMessage = await this.getUserFriendlyErrorMessage( + response.code || 'SERVER_ERROR', + response.message || 'Unknown error' + ); + + this.dialogService.openCriticalErrorDialog({ + code: response.code || 'SERVER_ERROR', + message: userMessage, + }); + } + + // Convert error code to user-friendly translated message + private async getUserFriendlyErrorMessage( + code: string, + _messageKey: string + ): Promise { + const errorKeyMapping: Record = { + NETWORK_TIMEOUT: 'TOASTS.ERRORS.NETWORK_TIMEOUT', + SERVER_ERROR: 'TOASTS.ERRORS.SERVER_ERROR', + AUTH_FAILED: 'TOASTS.ERRORS.AUTH_FAILED', + MALFORMED_REQUEST: 'TOASTS.ERRORS.MALFORMED_REQUEST', + RATE_LIMIT_EXCEEDED: 'TOASTS.ERRORS.RATE_LIMIT_EXCEEDED', + }; + + const translationKey = + errorKeyMapping[code] || 'TOASTS.ERRORS.UNEXPECTED_ERROR'; + + try { + return await this.utilsService.translate(translationKey); + } catch { + return 'An error occurred. Please try again.'; + } + } +} 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 3e616d1b0..fd00c0379 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 @@ -8,65 +8,15 @@ import { UtilsService } from '../utils/utils.service'; import { ToastrService } from 'ngx-toastr'; import { ToastService } from '../toast/toast.service'; import { DialogService } from '../dialog/dialog.service'; -import { - ValidationErrorResponse, - CriticalErrorResponse, - SuccessResponse, - OperationResponse, -} from './server-types'; import { ExportNodeProperties } from '@mmp/map/types'; import { createMockUtilsService } from '../../../../test/mocks/utils-service.mock'; import { Observable } from 'rxjs'; import { UserSettings } from '../../../shared/models/settings.model'; -import * as Y from 'yjs'; -import { - populateYMapFromNodeProps, - yMapToNodeProps, - buildYjsWsUrl, - parseWriteAccessBytes, - resolveClientColor, - findAffectedNodes, - resolveMmpPropertyUpdate, - resolveCompoundMmpUpdates, - sortParentFirst, -} from './yjs-utils'; - -// Mock the NodePropertyMapping module -jest.mock('@mmp/index', () => ({ - NodePropertyMapping: { - name: ['name'], - locked: ['locked'], - coordinates: ['coordinates'], - imageSrc: ['image', 'src'], - imageSize: ['image', 'size'], - linkHref: ['link', 'href'], - backgroundColor: ['colors', 'background'], - branchColor: ['colors', 'branch'], - fontWeight: ['font', 'weight'], - fontStyle: ['font', 'style'], - fontSize: ['font', 'size'], - nameColor: ['colors', 'name'], - hidden: ['hidden'], - }, -})); - -// Import NodePropertyMapping after mocking - needed for service to work -// eslint-disable-next-line @typescript-eslint/no-unused-vars -import { NodePropertyMapping } from '@mmp/index'; - -// Minimal private access for tests that must interact with Socket.io internals -interface MapSyncServiceSocketAccess { - socket: { - emit: jest.Mock; - removeAllListeners: jest.Mock; - on?: jest.Mock; - io?: { on: jest.Mock }; - }; - emitAddNode: (node: ExportNodeProperties) => void; -} +import { SyncStrategy } from './sync-strategy'; -function asSocketAccess(service: MapSyncService): MapSyncServiceSocketAccess { - return service as unknown as MapSyncServiceSocketAccess; +// Narrow accessor: only exposes the SyncStrategy interface, not sub-service internals +function getStrategy(service: MapSyncService): SyncStrategy { + return (service as unknown as { syncStrategy: SyncStrategy }).syncStrategy; } function createMockNode( @@ -93,67 +43,29 @@ function createMockNode( describe('MapSyncService', () => { let service: MapSyncService; let mmpService: jest.Mocked; - let httpService: jest.Mocked; - let storageService: jest.Mocked; let settingsService: jest.Mocked; - let utilsService: jest.Mocked; - let toastService: jest.Mocked; - let dialogService: jest.Mocked; - let toastrService: jest.Mocked; - - const mockNode: ExportNodeProperties = { - id: 'node-1', - name: 'Test 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, - }; + const mockNode = createMockNode({ id: 'node-1', name: 'Test Node' }); const mockMapSnapshot: ExportNodeProperties[] = [mockNode]; - const mockServerMap = { - uuid: 'test-uuid', - lastModified: new Date().toISOString(), - deletedAt: new Date(Date.now() + 86400000).toISOString(), - deleteAfterDays: 30, - data: mockMapSnapshot, - options: { fontMaxSize: 18, fontMinSize: 10, fontIncrement: 2 }, - createdAt: new Date().toISOString(), - }; - beforeEach(() => { - // Only mock methods that are actually used in these tests mmpService = { new: jest.fn(), selectNode: jest.fn(), getRootNode: jest.fn(), on: jest.fn(), updateNode: jest.fn(), + updateAdditionalMapOptions: jest.fn(), existNode: jest.fn().mockReturnValue(true), addNodesFromServer: jest.fn(), removeNode: jest.fn(), highlightNode: jest.fn(), exportAsJSON: jest.fn().mockReturnValue([]), + undo: jest.fn(), + redo: jest.fn(), + history: jest.fn().mockReturnValue({ snapshots: [], index: 0 }), } as unknown as jest.Mocked; - httpService = { - get: jest.fn(), - post: jest.fn(), - } as unknown as jest.Mocked; - - storageService = { - get: jest.fn(), - set: jest.fn(), - } as unknown as jest.Mocked; - settingsService = { getCachedUserSettings: jest.fn(), getCachedSystemSettings: jest.fn().mockReturnValue({ @@ -162,23 +74,6 @@ describe('MapSyncService', () => { setEditMode: jest.fn(), } as unknown as jest.Mocked; - toastService = { - showValidationCorrection: jest.fn(), - } as unknown as jest.Mocked; - - dialogService = { - openCriticalErrorDialog: jest.fn(), - } as unknown as jest.Mocked; - - toastrService = { - error: jest.fn(), - success: jest.fn(), - warning: jest.fn(), - } as unknown as jest.Mocked; - - // Create mock UtilsService using shared test utility - utilsService = createMockUtilsService(); - const subscribeMock = jest.fn().mockReturnValue({ unsubscribe: jest.fn() }); mmpService.on.mockReturnValue({ subscribe: subscribeMock, @@ -196,13 +91,32 @@ describe('MapSyncService', () => { providers: [ MapSyncService, { provide: MmpService, useValue: mmpService }, - { provide: HttpService, useValue: httpService }, - { provide: StorageService, useValue: storageService }, + { + 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: utilsService }, - { provide: ToastService, useValue: toastService }, - { provide: DialogService, useValue: dialogService }, - { provide: ToastrService, useValue: toastrService }, + { 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(), + }, + }, ], }); @@ -213,180 +127,9 @@ describe('MapSyncService', () => { service.ngOnDestroy(); }); - describe('operation response handling', () => { - let handleResponse: (response: OperationResponse) => Promise; - - beforeEach(() => { - const emitSpy = jest.fn( - ( - _event: string, - _data: unknown, - callback?: (response: OperationResponse) => Promise - ) => { - if (callback) handleResponse = callback; - } - ); - - const socketSpy = { - emit: emitSpy, - removeAllListeners: jest.fn(), - }; - asSocketAccess(service).socket = socketSpy; - jest.spyOn(service, 'getAttachedMap').mockReturnValue({ - key: 'map-test', - cachedMap: { - uuid: 'test-uuid', - data: mockMapSnapshot, - lastModified: Date.now(), - createdAt: Date.now(), - deletedAt: Date.now() + 86400000, - deleteAfterDays: 30, - options: { fontMaxSize: 18, fontMinSize: 10, fontIncrement: 2 }, - }, - }); - - asSocketAccess(service).emitAddNode(mockNode); - }); - - it('success response triggers no side effects', async () => { - const successResponse: SuccessResponse = { - success: true, - data: [mockNode], - }; - - await handleResponse(successResponse); - - expect({ - mapReloaded: mmpService.new.mock.calls.length, - toastShown: toastService.showValidationCorrection.mock.calls.length, - dialogOpened: dialogService.openCriticalErrorDialog.mock.calls.length, - }).toEqual({ mapReloaded: 0, toastShown: 0, dialogOpened: 0 }); - }); - - it('error with fullMapState reloads map', async () => { - const errorResponse: ValidationErrorResponse = { - success: false, - errorType: 'validation', - code: 'INVALID_PARENT', - message: 'Invalid parent', - fullMapState: mockServerMap, - }; - - await handleResponse(errorResponse); - - expect(mmpService.new).toHaveBeenCalledWith(mockMapSnapshot, false); - }); - - it('error with fullMapState shows toast', async () => { - const errorResponse: CriticalErrorResponse = { - success: false, - errorType: 'critical', - code: 'SERVER_ERROR', - message: 'Server error', - fullMapState: mockServerMap, - }; - - await handleResponse(errorResponse); - - expect(toastService.showValidationCorrection).toHaveBeenCalledWith( - 'add node', - 'Operation failed - map reloaded from server' - ); - }); - - it('error without fullMapState shows critical dialog', async () => { - const errorResponse: CriticalErrorResponse = { - success: false, - errorType: 'critical', - code: 'SERVER_ERROR', - message: 'Server error', - }; - - await handleResponse(errorResponse); - - expect(dialogService.openCriticalErrorDialog).toHaveBeenCalledWith({ - code: 'SERVER_ERROR', - message: expect.stringContaining('server encountered an error'), - }); - }); - - it('malformed response shows critical dialog', async () => { - const malformedResponse = { - invalid: 'response', - } as unknown as OperationResponse; - - await handleResponse(malformedResponse); - - expect(dialogService.openCriticalErrorDialog).toHaveBeenCalledWith({ - code: 'MALFORMED_RESPONSE', - message: expect.stringContaining('invalid response'), - }); - }); - - it('malformed response with translation failure uses fallback message', async () => { - // Simulate translation service failure - utilsService.translate.mockImplementation(async () => { - throw new Error('Translation failed'); - }); - - const malformedResponse = { - invalid: 'response', - } as unknown as OperationResponse; - - await handleResponse(malformedResponse); - - expect(dialogService.openCriticalErrorDialog).toHaveBeenCalledWith({ - code: 'MALFORMED_RESPONSE', - message: 'Invalid server response. Please try again.', - }); - }); - - it('error with malformed fullMapState shows critical dialog', async () => { - const errorResponse: ValidationErrorResponse = { - success: false, - errorType: 'validation', - code: 'INVALID_PARENT', - message: 'Invalid parent', - fullMapState: { - // Missing required fields - malformed - uuid: 'test-uuid', - data: [], - } as unknown as typeof mockServerMap, - }; - - await handleResponse(errorResponse); - - expect(dialogService.openCriticalErrorDialog).toHaveBeenCalledWith({ - code: 'MALFORMED_RESPONSE', - message: expect.stringContaining('invalid response'), - }); - }); - - it('error without fullMapState with translation failure uses fallback', async () => { - // Simulate translation service failure - utilsService.translate.mockImplementation(async () => { - throw new Error('Translation failed'); - }); - - const errorResponse: CriticalErrorResponse = { - success: false, - errorType: 'critical', - code: 'SERVER_ERROR', - message: 'Server error', - }; - - await handleResponse(errorResponse); - - expect(dialogService.openCriticalErrorDialog).toHaveBeenCalledWith({ - code: 'SERVER_ERROR', - message: 'An error occurred. Please try again.', - }); - }); - }); - describe('map initialization', () => { beforeEach(() => { - const mockCachedMapEntry = { + jest.spyOn(service, 'getAttachedMap').mockReturnValue({ key: 'map-test-uuid', cachedMap: { uuid: 'test-uuid', @@ -397,20 +140,12 @@ describe('MapSyncService', () => { deleteAfterDays: 30, options: { fontMaxSize: 18, fontMinSize: 10, fontIncrement: 2 }, }, - }; - - jest.spyOn(service, 'getAttachedMap').mockReturnValue(mockCachedMapEntry); - - const socketSpy = { - emit: jest.fn(), - on: jest.fn().mockReturnThis(), - removeAllListeners: jest.fn(), - io: { - on: jest.fn().mockReturnThis(), - }, - }; + }); - asSocketAccess(service).socket = socketSpy; + // Prevent strategy from performing real I/O + jest + .spyOn(getStrategy(service), 'initMap') + .mockImplementation(() => undefined); }); it('loads map data into mmpService on initMap', () => { @@ -433,403 +168,118 @@ describe('MapSyncService', () => { expect(mmpService.selectNode).toHaveBeenCalledWith('root'); }); }); -}); - -// ─── Yjs utility function tests (pure functions) ────────────── - -describe('Y.Doc conversion utilities', () => { - let doc: Y.Doc; - let nodesMap: Y.Map>; - - beforeEach(() => { - doc = new Y.Doc(); - nodesMap = doc.getMap('nodes') as Y.Map>; - }); - afterEach(() => { - doc.destroy(); - }); + describe('undo and redo', () => { + it('undo delegates through to mmpService', () => { + service.undo(); - it('round-trips node properties through Y.Map', () => { - const input = createMockNode({ - id: 'n1', - name: 'Hello', - parent: 'root', - k: 1.5, - isRoot: false, - locked: true, - detached: true, - coordinates: { x: 100, y: 200 }, - colors: { name: '#ff0000', background: '#00ff00', branch: '#0000ff' }, - font: { size: 16, style: 'italic', weight: 'bold' }, - image: { src: 'http://img.png', size: 50 }, - link: { href: 'http://example.com' }, + expect(mmpService.undo).toHaveBeenCalled(); }); - const yNode = new Y.Map(); - populateYMapFromNodeProps(yNode, input); - nodesMap.set('n1', yNode); - - const result = yMapToNodeProps(nodesMap.get('n1')!); - - expect(result).toEqual( - expect.objectContaining({ - id: 'n1', - name: 'Hello', - parent: 'root', - k: 1.5, - isRoot: false, - locked: true, - detached: true, - coordinates: { x: 100, y: 200 }, - colors: { - name: '#ff0000', - background: '#00ff00', - branch: '#0000ff', - }, - font: { size: 16, style: 'italic', weight: 'bold' }, - image: { src: 'http://img.png', size: 50 }, - link: { href: 'http://example.com' }, - }) - ); - }); - - it('applies defaults for missing optional properties', () => { - const input: ExportNodeProperties = { - id: 'n2', - parent: undefined, - k: undefined, - name: undefined, - isRoot: undefined, - locked: undefined, - detached: undefined, - coordinates: undefined, - colors: undefined, - font: undefined, - image: undefined, - link: undefined, - } as unknown as ExportNodeProperties; - - const yNode = new Y.Map(); - populateYMapFromNodeProps(yNode, input); - nodesMap.set('n2', yNode); - - const result = yMapToNodeProps(nodesMap.get('n2')!); - - expect(result).toEqual( - expect.objectContaining({ - parent: null, - k: 1, - name: '', - isRoot: false, - locked: false, - detached: false, - coordinates: { x: 0, y: 0 }, - }) - ); - }); -}); - -describe('write access message parsing', () => { - it('returns true for writable message', () => { - const result = parseWriteAccessBytes(new Uint8Array([4, 1])); - expect(result).toBe(true); - }); - - it('returns false for read-only message', () => { - const result = parseWriteAccessBytes(new Uint8Array([4, 0])); - expect(result).toBe(false); - }); - - it('returns null for wrong type byte', () => { - const result = parseWriteAccessBytes(new Uint8Array([0, 1])); - expect(result).toBeNull(); - }); - - it('returns null for message too short to be valid', () => { - const result = parseWriteAccessBytes(new Uint8Array([4])); - expect(result).toBeNull(); - }); -}); - -describe('Yjs URL building', () => { - let querySelectorSpy: jest.SpyInstance; - - beforeEach(() => { - querySelectorSpy = jest.spyOn(document, 'querySelector'); - }); - - afterEach(() => { - querySelectorSpy.mockRestore(); - }); + it('redo delegates through to mmpService', () => { + service.redo(); - // jsdom default location is http://localhost, so tests use that baseline - it('builds ws URL and uses document base href', () => { - querySelectorSpy.mockReturnValue({ - getAttribute: () => '/', + expect(mmpService.redo).toHaveBeenCalled(); }); - - const url = buildYjsWsUrl(); - - // jsdom runs on http://localhost -> ws: - expect(url).toBe('ws://localhost/yjs'); }); - it('incorporates base href into path', () => { - querySelectorSpy.mockReturnValue({ - getAttribute: () => '/app/', - }); - - const url = buildYjsWsUrl(); + describe('prepareExistingMap', () => { + let httpService: jest.Mocked; - expect(url).toBe('ws://localhost/app/yjs'); - }); + const mockServerMap = { + uuid: 'test-uuid', + lastModified: '2026-01-01', + deletedAt: '2026-02-01', + deleteAfterDays: 30, + data: [createMockNode({ id: 'root', isRoot: true })], + options: { fontMaxSize: 18, fontMinSize: 10, fontIncrement: 2 }, + createdAt: '2026-01-01', + }; - it('appends trailing slash to base href if missing', () => { - querySelectorSpy.mockReturnValue({ - getAttribute: () => '/app', + beforeEach(() => { + httpService = TestBed.inject(HttpService) as jest.Mocked; }); - const url = buildYjsWsUrl(); + it('appends secret param to HTTP request when secret is set', async () => { + httpService.get.mockResolvedValue({ + ok: true, + json: () => Promise.resolve({ ...mockServerMap, writable: true }), + } as unknown as Response); - expect(url).toBe('ws://localhost/app/yjs'); - }); - - it('defaults base href to / when no base element', () => { - querySelectorSpy.mockReturnValue(null); - - const url = buildYjsWsUrl(); + await service.prepareExistingMap('test-uuid', 'my-secret'); - expect(url).toBe('ws://localhost/yjs'); - }); - - it('selects protocol based on page protocol', () => { - // Verify the protocol-selection logic via the method output - // jsdom defaults to http: -> ws:, confirming the mapping works - querySelectorSpy.mockReturnValue(null); - const url = buildYjsWsUrl(); - expect(url).toMatch(/^ws:\/\//); - // The https: -> wss: path uses the same ternary expression - }); -}); - -describe('Y.Doc property application to MMP', () => { - it('resolves simple property (name) via reverse mapping', () => { - const updates = resolveMmpPropertyUpdate('name', 'New Name'); - expect(updates).toEqual([{ prop: 'name', val: 'New Name' }]); - }); - - it('resolves simple property (locked) via reverse mapping', () => { - const updates = resolveMmpPropertyUpdate('locked', true); - expect(updates).toEqual([{ prop: 'locked', val: true }]); - }); - - it('resolves compound property (colors) via reverse mapping', () => { - const updates = resolveMmpPropertyUpdate('colors', { - background: '#ff0000', - branch: '#00ff00', - name: '#0000ff', - }); - - expect(updates).toEqual([ - { prop: 'backgroundColor', val: '#ff0000' }, - { prop: 'branchColor', val: '#00ff00' }, - { prop: 'nameColor', val: '#0000ff' }, - ]); - }); - - it('resolves compound property (font) via reverse mapping', () => { - const updates = resolveMmpPropertyUpdate('font', { - size: 20, - weight: 'bold', - style: 'italic', + expect(httpService.get).toHaveBeenCalledWith( + expect.anything(), + expect.stringContaining('?secret=my-secret') + ); }); - expect(updates).toEqual( - expect.arrayContaining([ - { prop: 'fontSize', val: 20 }, - { prop: 'fontWeight', val: 'bold' }, - { prop: 'fontStyle', val: 'italic' }, - ]) - ); - }); - - it('returns empty array for unknown property keys', () => { - const updates = resolveMmpPropertyUpdate('unknown_key', 'value'); - expect(updates).toEqual([]); - }); - - it('handles null compound value gracefully', () => { - const updates = resolveCompoundMmpUpdates( - { background: 'backgroundColor' }, - null as unknown as Record - ); - expect(updates).toEqual([]); - }); -}); - -describe('client color resolution', () => { - it('returns existing color when no collision', () => { - const result = resolveClientColor( - '#ff0000', - new Set(['#00ff00', '#0000ff']) - ); - expect(result).toBe('#ff0000'); - }); - - it('generates a different valid hex color on collision', () => { - const result = resolveClientColor('#00ff00', new Set(['#00ff00'])); - expect(result).toMatch(/^#(?!00ff00)[0-9a-f]{6}$/); - }); - - it('handles empty used colors set', () => { - const result = resolveClientColor('#ff0000', new Set()); - expect(result).toBe('#ff0000'); - }); -}); - -describe('findAffectedNodes', () => { - it('collects node IDs from both old and new mappings', () => { - const oldMapping = { - c1: { nodeId: 'node-a', color: '#ff0000' }, - c2: { nodeId: 'node-b', color: '#00ff00' }, - }; - - const newMapping = { - c1: { nodeId: 'node-b', color: '#ff0000' }, - c3: { nodeId: 'node-c', color: '#0000ff' }, - }; - - const result = findAffectedNodes(oldMapping, newMapping); - - expect(result).toEqual(new Set(['node-a', 'node-b', 'node-c'])); - }); - - it('excludes empty nodeId strings', () => { - const oldMapping = { - c1: { nodeId: '', color: '#ff0000' }, - }; - - const newMapping = { - c1: { nodeId: 'node-a', color: '#ff0000' }, - }; - - const result = findAffectedNodes(oldMapping, newMapping); + it('omits secret param when modification secret is empty', async () => { + httpService.get.mockResolvedValue({ + ok: true, + json: () => Promise.resolve({ ...mockServerMap, writable: true }), + } as unknown as Response); - expect(result).toEqual(new Set(['node-a'])); - }); + await service.prepareExistingMap('test-uuid', ''); - it('returns empty set when no nodes selected', () => { - const oldMapping = { - c1: { nodeId: '', color: '#ff0000' }, - }; - - const newMapping = { - c1: { nodeId: '', color: '#ff0000' }, - }; + expect(httpService.get).toHaveBeenCalledWith( + expect.anything(), + '/maps/test-uuid' + ); + }); - const result = findAffectedNodes(oldMapping, newMapping); + it('sets writable true on strategy when response writable is true', async () => { + httpService.get.mockResolvedValue({ + ok: true, + json: () => Promise.resolve({ ...mockServerMap, writable: true }), + } as unknown as Response); + const setWritableSpy = jest.spyOn(getStrategy(service), 'setWritable'); - expect(result.size).toBe(0); - }); -}); + await service.prepareExistingMap('test-uuid', 'secret'); -describe('sortParentFirst', () => { - it('places root node first when children appear before parent', () => { - const child = createMockNode({ - id: 'child-1', - parent: 'root-1', - isRoot: false, + expect(setWritableSpy).toHaveBeenCalledWith(true); }); - const root = createMockNode({ id: 'root-1', parent: '', isRoot: true }); - const result = sortParentFirst([child, root]); + it('sets writable false on strategy when response writable is false', async () => { + httpService.get.mockResolvedValue({ + ok: true, + json: () => Promise.resolve({ ...mockServerMap, writable: false }), + } as unknown as Response); + const setWritableSpy = jest.spyOn(getStrategy(service), 'setWritable'); - expect(result.map(n => n.id)).toEqual(['root-1', 'child-1']); - }); + await service.prepareExistingMap('test-uuid', 'wrong'); - it('ensures grandchild nodes come after their parent', () => { - const grandchild = createMockNode({ - id: 'gc-1', - parent: 'child-1', - isRoot: false, - }); - const child = createMockNode({ - id: 'child-1', - parent: 'root-1', - isRoot: false, + expect(setWritableSpy).toHaveBeenCalledWith(false); }); - const root = createMockNode({ id: 'root-1', parent: '', isRoot: true }); - const result = sortParentFirst([grandchild, child, root]); + it('defaults to writable true when response writable is undefined', async () => { + httpService.get.mockResolvedValue({ + ok: true, + json: () => Promise.resolve(mockServerMap), + } as unknown as Response); + const setWritableSpy = jest.spyOn(getStrategy(service), 'setWritable'); - expect(result.map(n => n.id)).toEqual(['root-1', 'child-1', 'gc-1']); - }); + await service.prepareExistingMap('test-uuid', ''); - it('handles already-sorted input without changing order', () => { - const root = createMockNode({ id: 'root-1', parent: '', isRoot: true }); - const child1 = createMockNode({ - id: 'c1', - parent: 'root-1', - isRoot: false, + expect(setWritableSpy).toHaveBeenCalledWith(true); }); - const child2 = createMockNode({ - id: 'c2', - parent: 'root-1', - isRoot: false, - }); - - const result = sortParentFirst([root, child1, child2]); - - expect(result.map(n => n.id)).toEqual(['root-1', 'c1', 'c2']); }); - it('returns original array when no root is found', () => { - const node1 = createMockNode({ id: 'n1', parent: 'n2', isRoot: false }); - const node2 = createMockNode({ id: 'n2', parent: 'n1', isRoot: false }); + describe('lifecycle', () => { + it('ngOnDestroy calls destroy on strategy', () => { + const destroySpy = jest.spyOn(getStrategy(service), 'destroy'); - const result = sortParentFirst([node1, node2]); + service.ngOnDestroy(); - expect(result.map(n => n.id)).toEqual(['n1', 'n2']); - }); - - it('groups sibling nodes under their shared parent', () => { - const root = createMockNode({ id: 'root', parent: '', isRoot: true }); - const b = createMockNode({ id: 'b', parent: 'root', isRoot: false }); - const a = createMockNode({ id: 'a', parent: 'root', isRoot: false }); - const bChild = createMockNode({ - id: 'b-child', - parent: 'b', - isRoot: false, + expect(destroySpy).toHaveBeenCalled(); }); - const result = sortParentFirst([bChild, a, b, root]); - - expect(result[0].id).toBe('root'); - expect(result.indexOf(b)).toBeLessThan(result.indexOf(bChild)); - }); - - it('returns empty array for empty input', () => { - const result = sortParentFirst([]); + it('reset calls destroy on strategy', () => { + const destroySpy = jest.spyOn(getStrategy(service), 'destroy'); - expect(result).toEqual([]); - }); + service.reset(); - it('appends orphaned nodes not reachable from root', () => { - const root = createMockNode({ id: 'root', parent: '', isRoot: true }); - const child = createMockNode({ - id: 'child', - parent: 'root', - isRoot: false, + expect(destroySpy).toHaveBeenCalled(); }); - const orphan = createMockNode({ - id: 'orphan', - parent: 'deleted-parent', - isRoot: false, - }); - - const result = sortParentFirst([orphan, child, root]); - - expect(result.map(n => n.id)).toEqual(['root', 'child', 'orphan']); }); }); 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 8e55ef6b9..66c7b3f75 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 @@ -1,6 +1,6 @@ import { Injectable, OnDestroy, inject } from '@angular/core'; import { MmpService } from '../mmp/mmp.service'; -import { BehaviorSubject, Observable, Subscription } from 'rxjs'; +import { BehaviorSubject, Observable } from 'rxjs'; import { CachedAdminMapEntry, CachedAdminMapValue, @@ -8,63 +8,24 @@ import { CachedMapEntry, CachedMapOptions, } from '../../../shared/models/cached-map.model'; -import { io, Socket } from 'socket.io-client'; -import { NodePropertyMapping } from '@mmp/index'; -import { - ExportNodeProperties, - MapCreateEvent, - MapProperties, - NodeUpdateEvent, -} from '@mmp/map/types'; -import { - PrivateServerMap, - ResponseMapOptionsUpdated, - ResponseMapUpdated, - ResponseNodeRemoved, - ResponseNodeUpdated, - ResponseNodesAdded, - ResponseSelectionUpdated, - ResponseClientNotification, - ServerMap, - ServerMapInfo, - ResponseUndoRedoChanges, - ReversePropertyMapping, - OperationResponse, - ValidationErrorResponse, - CriticalErrorResponse, -} from './server-types'; +import { ExportNodeProperties, MapProperties } from '@mmp/map/types'; +import { PrivateServerMap, ServerMap, ServerMapInfo } from './server-types'; import { API_URL, HttpService } from '../../http/http.service'; import { COLORS } from '../mmp/mmp-utils'; import { UtilsService } from '../utils/utils.service'; import { StorageService } from '../storage/storage.service'; import { SettingsService } from '../settings/settings.service'; import { ToastrService } from 'ngx-toastr'; -import { MapDiff, SnapshotChanges } from '@mmp/map/handlers/history'; import { ToastService } from '../toast/toast.service'; import { DialogService } from '../dialog/dialog.service'; -import * as Y from 'yjs'; -import { WebsocketProvider } from 'y-websocket'; -import { - ClientColorMapping, - ClientColorMappingValue, - populateYMapFromNodeProps, - yMapToNodeProps, - buildYjsWsUrl, - parseWriteAccessBytes, - resolveClientColor, - findAffectedNodes, - resolveMmpPropertyUpdate, - sortParentFirst, -} from './yjs-utils'; - -const DEFAULT_COLOR = '#000000'; -const DEFAULT_SELF_COLOR = '#c0c0c0'; -const WS_CLOSE_MAP_DELETED = 4001; -const MESSAGE_WRITE_ACCESS = 4; +import { ClientColorMapping, ClientColorMappingValue } from './yjs-utils'; +import { MapSyncErrorHandler } from './map-sync-error-handler'; +import { MapSyncContext, ConnectionStatus } from './map-sync-context'; +import { SocketIoSyncService } from './socket-io-sync.service'; +import { YjsSyncService } from './yjs-sync.service'; +import { SyncStrategy } from './sync-strategy'; -type ServerClientList = Record; - -export type ConnectionStatus = 'connected' | 'disconnected' | null; +export { ConnectionStatus } from './map-sync-context'; @Injectable({ providedIn: 'root', @@ -74,8 +35,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); @@ -87,33 +48,24 @@ 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; - - // Yjs fields (used when yjs feature flag is true) - private yDoc: Y.Doc | null = null; - private wsProvider: WebsocketProvider | null = null; - private yjsSynced = false; - private yjsWritable = false; - private yjsSubscriptions: Subscription[] = []; - private yjsMapId: string | null = null; - private yjsNodesObserver: - | Parameters['observeDeep']>[0] - | null = null; - private yjsOptionsObserver: Parameters['observe']>[0] | null = - null; - private yjsAwarenessHandler: (() => void) | null = null; + // Sync strategy (only one instantiated based on feature flag) + private readonly syncStrategy: SyncStrategy; // Common fields private colorMapping: ClientColorMapping; private availableColors: string[]; private clientColor: string; private modificationSecret: string; - private readonly yjsEnabled: boolean; constructor() { - // Initialization of the behavior subjects. this.attachedMapSubject = new BehaviorSubject(null); this.attachedNodeSubject = new BehaviorSubject( null @@ -128,29 +80,47 @@ export class MapSyncService implements OnDestroy { ]; this.modificationSecret = ''; this.colorMapping = {}; - this.yjsEnabled = + const yjsEnabled = this.settingsService.getCachedSystemSettings()?.featureFlags?.yjs ?? false; - if (!this.yjsEnabled) { - this.initSocketConnection(); + const ctx = this.createContext(); + + if (yjsEnabled) { + this.syncStrategy = new YjsSyncService( + ctx, + this.mmpService, + this.settingsService, + this.utilsService, + this.toastrService, + this.httpService + ); + } else { + const errorHandler = new MapSyncErrorHandler( + this.mmpService, + this.utilsService, + this.toastService, + this.dialogService + ); + this.syncStrategy = new SocketIoSyncService( + ctx, + this.mmpService, + this.settingsService, + this.utilsService, + this.toastrService, + errorHandler + ); } + + this.syncStrategy.connect(); } ngOnDestroy() { - if (this.yjsEnabled) { - this.resetYjs(); - } else { - this.resetSocketIo(); - } + this.syncStrategy.destroy(); } // ─── Public API ────────────────────────────────────────────── - /** - * Creates a new map on server and prepares it locally - * Stores admin data and enables edit mode - */ public async prepareNewMap(): Promise { const privateServerMap: PrivateServerMap = await this.postMapToServer(); this.storePrivateMapData(privateServerMap); @@ -169,19 +139,14 @@ export class MapSyncService implements OnDestroy { return; } + this.syncStrategy.setWritable(serverMap.writable !== false); this.updateCachedMapForAdmin(serverMap); this.prepareMap(serverMap); return serverMap; } - // Detach MMP listeners but keep the Yjs connection alive for reuse public reset() { - if (this.yjsEnabled) { - this.detachYjsObservers(); - this.unsubscribeYjsListeners(); - } else { - this.resetSocketIo(); - } + this.syncStrategy.destroy(); this.colorMapping = {}; } @@ -190,13 +155,7 @@ export class MapSyncService implements OnDestroy { this.attachedNodeSubject.next( this.mmpService.selectNode(this.mmpService.getRootNode().id) ); - - if (this.yjsEnabled) { - this.initYjs(); - } else { - this.createSocketIoListeners(); - this.listenServerEvents(this.getAttachedMap().cachedMap.uuid); - } + this.syncStrategy.initMap(this.getAttachedMap().cachedMap.uuid); } public attachMap(cachedMapEntry: CachedMapEntry): void { @@ -227,7 +186,6 @@ export class MapSyncService implements OnDestroy { return this.connectionStatusSubject.getValue(); } - // update the attached map from outside control flow public async updateAttachedMap(): Promise { const cachedMapEntry: CachedMapEntry = this.getAttachedMap(); @@ -244,20 +202,20 @@ export class MapSyncService implements OnDestroy { this.attachMap({ key: cachedMapEntry.key, cachedMap }); } + public undo(): void { + this.syncStrategy.undo(); + } + + public redo(): void { + this.syncStrategy.redo(); + } + public updateMapOptions(options?: CachedMapOptions) { - if (this.yjsEnabled) { - this.writeMapOptionsToYDoc(options); - } else { - this.emitUpdateMapOptions(options); - } + this.syncStrategy.updateMapOptions(options); } public async deleteMap(adminId: string): Promise { - if (this.yjsEnabled) { - await this.deleteMapViaHttp(adminId); - } else { - this.deleteMapViaSocket(adminId); - } + await this.syncStrategy.deleteMap(adminId); } public async fetchUserMapsFromServer(): Promise { @@ -276,1494 +234,34 @@ export class MapSyncService implements OnDestroy { })); } - // ─── Socket.io initialization ──────────────────────────────── - - private initSocketConnection(): void { - const reconnectOptions = { - reconnection: true, - reconnectionDelay: 1000, - reconnectionDelayMax: 5000, - reconnectionAttempts: 60, - randomizationFactor: 0.5, - }; - - const baseHref = - document.querySelector('base')?.getAttribute('href') ?? '/'; - this.socket = - baseHref !== '/' - ? io('', { - path: `${baseHref}socket.io`, - ...reconnectOptions, - }) - : io({ - ...reconnectOptions, - }); - } - - private resetSocketIo(): void { - if (this.socket) { - this.socket.removeAllListeners(); - this.leaveMap(); - } - } - - // ─── Socket.io map listeners (MMP → Socket.io) ────────────── - - private createSocketIoListeners() { - this.setupSocketIoCreateHandler(); - this.setupSocketIoSelectionHandlers(); - this.setupSocketIoNodeUpdateHandler(); - this.setupSocketIoUndoRedoHandlers(); - this.setupSocketIoNodeCreateHandler(); - this.setupSocketIoPasteHandler(); - this.setupSocketIoNodeRemoveHandler(); - } - - private setupSocketIoCreateHandler(): void { - this.mmpService.on('create').subscribe((_result: MapCreateEvent) => { - this.attachedNodeSubject.next(this.mmpService.selectNode()); - this.updateAttachedMap(); - this.updateMap(); - }); - } - - private setupSocketIoSelectionHandlers(): void { - this.mmpService - .on('nodeSelect') - .subscribe((nodeProps: ExportNodeProperties) => { - this.updateNodeSelectionSocketIo(nodeProps.id, true); - this.attachedNodeSubject.next(nodeProps); - }); - - this.mmpService - .on('nodeDeselect') - .subscribe((nodeProps: ExportNodeProperties) => { - this.updateNodeSelectionSocketIo(nodeProps.id, false); - this.attachedNodeSubject.next(nodeProps); - }); - } - - private setupSocketIoNodeUpdateHandler(): void { - this.mmpService.on('nodeUpdate').subscribe((result: NodeUpdateEvent) => { - this.attachedNodeSubject.next(result.nodeProperties); - this.emitUpdateNode(result); - this.updateAttachedMap(); - }); - } - - private setupSocketIoUndoRedoHandlers(): void { - this.mmpService.on('undo').subscribe((diff?: MapDiff) => { - this.attachedNodeSubject.next(this.mmpService.selectNode()); - this.updateAttachedMap(); - this.emitApplyMapChangesByDiff(diff, 'undo'); - }); - - this.mmpService.on('redo').subscribe((diff?: MapDiff) => { - this.attachedNodeSubject.next(this.mmpService.selectNode()); - this.updateAttachedMap(); - this.emitApplyMapChangesByDiff(diff, 'redo'); - }); - } - - private setupSocketIoNodeCreateHandler(): void { - this.mmpService - .on('nodeCreate') - .subscribe((newNode: ExportNodeProperties) => { - this.emitAddNode(newNode); - this.updateAttachedMap(); - this.mmpService.selectNode(newNode.id); - this.mmpService.editNode(); - }); - } + // ─── Context factory ───────────────────────────────────────── - private setupSocketIoPasteHandler(): void { - this.mmpService - .on('nodePaste') - .subscribe((newNodes: ExportNodeProperties[]) => { - this.emitAddNodes(newNodes); - this.updateAttachedMap(); - }); - } - - private setupSocketIoNodeRemoveHandler(): void { - this.mmpService - .on('nodeRemove') - .subscribe((removedNode: ExportNodeProperties) => { - this.emitRemoveNode(removedNode); - this.updateAttachedMap(); - }); - } - - // ─── Socket.io emit methods ────────────────────────────────── - - private async joinMap( - mmpUuid: string, - color: string - ): Promise { - return new Promise((resolve, reject) => { - this.socket.emit( - 'join', - { mapId: mmpUuid, color }, - (serverMap: MapProperties) => { - if (!serverMap) { - reject('Server Map not available'); - return; - } - resolve(serverMap); - } - ); - }); - } - - private leaveMap() { - this.socket.emit('leave'); - } - - private emitAddNode(newNode: ExportNodeProperties) { - // Emit with acknowledgment callback for error handling - this.socket.emit( - 'addNodes', - { - mapId: this.getAttachedMap().cachedMap.uuid, - nodes: [newNode], - modificationSecret: this.modificationSecret, - }, - async (response: OperationResponse) => { - await this.handleOperationResponse(response, 'add node'); - } - ); - } - - /** - * Add multiple nodes with server validation and error handling - * Used for bulk operations like paste - */ - private emitAddNodes(newNodes: ExportNodeProperties[]) { - // Emit with acknowledgment callback for error handling - this.socket.emit( - 'addNodes', - { - mapId: this.getAttachedMap().cachedMap.uuid, - nodes: newNodes, - modificationSecret: this.modificationSecret, - }, - async (response: OperationResponse) => { - await this.handleOperationResponse(response, 'add nodes'); - } - ); - } - - /** - * Update node property with server validation and error handling - */ - private emitUpdateNode(nodeUpdate: NodeUpdateEvent) { - // Emit with acknowledgment callback for error handling - this.socket.emit( - 'updateNode', - { - mapId: this.getAttachedMap().cachedMap.uuid, - node: nodeUpdate.nodeProperties, - updatedProperty: nodeUpdate.changedProperty, - modificationSecret: this.modificationSecret, - }, - async (response: OperationResponse) => { - await this.handleOperationResponse(response, 'update node'); - } - ); - } - - /** - * Remove node with server validation and error handling - */ - private emitRemoveNode(removedNode: ExportNodeProperties) { - // Emit with acknowledgment callback for error handling - this.socket.emit( - 'removeNode', - { - mapId: this.getAttachedMap().cachedMap.uuid, - node: removedNode, - modificationSecret: this.modificationSecret, - }, - async (response: OperationResponse) => { - await this.handleOperationResponse(response, 'remove node'); - } - ); - } - - /** - * Apply undo/redo changes with server validation and error handling - */ - private emitApplyMapChangesByDiff( - diff: MapDiff, - operationType: 'undo' | 'redo' - ) { - // Emit with acknowledgment callback for error handling - this.socket.emit( - 'applyMapChangesByDiff', - { - mapId: this.getAttachedMap().cachedMap.uuid, - diff, - modificationSecret: this.modificationSecret, + private createContext(): MapSyncContext { + return { + getAttachedMap: () => this.getAttachedMap(), + getModificationSecret: () => this.modificationSecret, + getColorMapping: () => this.colorMapping, + getClientColor: () => this.clientColor, + colorForNode: (nodeId: string) => this.colorForNode(nodeId), + setConnectionStatus: (status: ConnectionStatus) => + this.connectionStatusSubject.next(status), + setColorMapping: (mapping: ClientColorMapping) => { + this.colorMapping = mapping; }, - async (response: OperationResponse) => { - await this.handleOperationResponse( - response, - `${operationType} operation` - ); - } - ); - } - - private updateMap() { - const cachedMapEntry: CachedMapEntry = this.getAttachedMap(); - this.socket.emit('updateMap', { - mapId: cachedMapEntry.cachedMap.uuid, - map: cachedMapEntry.cachedMap, - modificationSecret: this.modificationSecret, - }); - } - - private emitUpdateMapOptions(options?: CachedMapOptions) { - const cachedMapEntry: CachedMapEntry = this.getAttachedMap(); - this.socket.emit('updateMapOptions', { - mapId: cachedMapEntry.cachedMap.uuid, - options, - modificationSecret: this.modificationSecret, - }); - } - - private deleteMapViaSocket(adminId: string): void { - const cachedMapEntry: CachedMapEntry = this.getAttachedMap(); - this.socket.emit('deleteMap', { - adminId, - mapId: cachedMapEntry.cachedMap.uuid, - }); - } - - // Remember all clients selections with the dedicated colors to switch between colors when clients change among nodes - private updateNodeSelectionSocketIo(id: string, selected: boolean) { - if (selected) { - this.colorMapping[this.socket.id] = { - color: DEFAULT_SELF_COLOR, - nodeId: id, - }; - } else { - this.colorMapping[this.socket.id] = { - color: DEFAULT_SELF_COLOR, - nodeId: '', - }; - const colorForNode: string = this.colorForNode(id); - if (colorForNode !== '') - this.mmpService.highlightNode(id, colorForNode, false); - } - - this.socket.emit('updateNodeSelection', { - mapId: this.getAttachedMap().cachedMap.uuid, - nodeId: id, - selected, - }); - } - - private checkModificationSecret() { - this.socket.emit( - 'checkModificationSecret', - { - mapId: this.getAttachedMap().cachedMap.uuid, - modificationSecret: this.modificationSecret, - }, - (result: boolean) => this.settingsService.setEditMode(result) - ); - } - - // ─── Socket.io server event handlers ───────────────────────── - - /** - * Setup all server event listeners and join the map - * Orchestrates socket event handlers for real-time collaboration - */ - private listenServerEvents(uuid: string): Promise { - this.checkModificationSecret(); - this.setupReconnectionHandler(uuid); - this.setupNotificationHandlers(); - this.setupNodeEventHandlers(); - this.setupMapEventHandlers(); - this.setupClientEventHandlers(); - this.setupConnectionEventHandlers(); - - return this.joinMap(uuid, this.clientColor); - } - - /** - * Setup socket reconnection handler - * Re-joins map and syncs state on reconnect - */ - private setupReconnectionHandler(uuid: string): void { - this.socket.io.on('reconnect', async () => { - const serverMap: MapProperties = await this.joinMap( - uuid, - this.clientColor - ); - this.setConnectionStatusSubject('connected'); - this.mmpService.new(serverMap.data, false); - }); - } - - /** - * Setup notification event handlers - * Displays toasts for client notifications from server - */ - private setupNotificationHandlers(): void { - this.socket.on( - 'clientNotification', - async (notification: ResponseClientNotification) => { - if (notification.clientId === this.socket.id) return; - await this.handleClientNotification(notification); - } - ); - } - - /** - * Handle individual client notification - * Translates message and shows appropriate toast type - */ - private async handleClientNotification( - notification: ResponseClientNotification - ): Promise { - const msg = await this.utilsService.translate(notification.message); - if (!msg) return; - - const toastHandlers: Record void> = { - error: () => this.toastrService.error(msg), - success: () => this.toastrService.success(msg), - warning: () => this.toastrService.warning(msg), - }; - toastHandlers[notification.type]?.(); - } - - /** - * Setup node-related event handlers - * Handles node additions, updates, and removals - */ - private setupNodeEventHandlers(): void { - this.setupNodesAddedHandler(); - this.setupNodeUpdatedHandler(); - this.setupNodeRemovedHandler(); - } - - /** - * Setup handler for nodes being added - */ - private setupNodesAddedHandler(): void { - this.socket.on('nodesAdded', (result: ResponseNodesAdded) => { - if (result.clientId === this.socket.id) return; - this.mmpService.addNodesFromServer(result.nodes); - }); - } - - /** - * Setup handler for node property updates - */ - private setupNodeUpdatedHandler(): void { - this.socket.on('nodeUpdated', (result: ResponseNodeUpdated) => { - if (result.clientId === this.socket.id) return; - this.handleNodeUpdate(result); - }); - } - - /** - * Handle individual node property update - */ - private handleNodeUpdate(result: ResponseNodeUpdated): void { - const newNode = result.node; - const existingNode = this.mmpService.getNode(newNode.id); - const propertyPath = NodePropertyMapping[result.property]; - const changedValue = UtilsService.get(newNode, propertyPath); - - this.mmpService.updateNode( - result.property, - changedValue, - false, - true, - existingNode.id - ); - } - - /** - * Setup handler for node removal - */ - private setupNodeRemovedHandler(): void { - this.socket.on('nodeRemoved', (result: ResponseNodeRemoved) => { - if (result.clientId === this.socket.id) return; - if (this.mmpService.existNode(result.nodeId)) { - this.mmpService.removeNode(result.nodeId, false); - } - }); - } - - /** - * Setup map-related event handlers - * Handles map updates, undo/redo, and options - */ - private setupMapEventHandlers(): void { - this.setupMapUpdatedHandler(); - this.setupMapChangesHandler(); - this.setupMapOptionsHandler(); - this.setupMapDeletedHandler(); - } - - /** - * Setup handler for full map updates - */ - private setupMapUpdatedHandler(): void { - this.socket.on('mapUpdated', (result: ResponseMapUpdated) => { - if (result.clientId === this.socket.id) return; - this.mmpService.new(result.map.data, false); - }); - } - - /** - * Setup handler for map undo/redo changes - */ - private setupMapChangesHandler(): void { - this.socket.on('mapChangesUndoRedo', (result: ResponseUndoRedoChanges) => { - if (result.clientId === this.socket.id) return; - this.applyMapDiff(result.diff); - }); - } - - /** - * Apply map diff for undo/redo operations - */ - private applyMapDiff(diff: MapDiff): void { - this.applyAddedNodes(diff.added); - this.applyUpdatedNodes(diff.updated); - this.applyDeletedNodes(diff.deleted); - } - - /** - * Apply added nodes from diff - */ - private applyAddedNodes(added: SnapshotChanges): void { - if (!added) return; - for (const nodeId in added) { - this.mmpService.addNode(added[nodeId], false); - } - } - - /** - * Apply updated nodes from diff - */ - private applyUpdatedNodes(updated: SnapshotChanges): void { - if (!updated) return; - for (const nodeId in updated) { - if (this.mmpService.existNode(nodeId)) { - this.applyNodePropertyUpdates(nodeId, updated[nodeId]); - } - } - } - - /** - * Apply property updates to a single node - */ - private applyNodePropertyUpdates( - nodeId: string, - nodeUpdates: Partial - ): void { - if (!nodeUpdates) return; - - for (const property in nodeUpdates) { - const updatedProperty = this.getClientProperty( - property, - (nodeUpdates as Record)[property] - ); - if (updatedProperty) { - this.mmpService.updateNode( - updatedProperty.clientProperty, - updatedProperty.directValue, - false, - true, - nodeId - ); - } - } - } - - /** - * Convert server property to client property format - */ - private getClientProperty( - serverProperty: string, - value: unknown - ): { clientProperty: string; directValue: unknown } | undefined { - const mapping = - ReversePropertyMapping[ - serverProperty as keyof typeof ReversePropertyMapping - ]; - - if (typeof mapping === 'string') { - return { clientProperty: mapping, directValue: value }; - } - - if (mapping && typeof value === 'object') { - const subProperty = Object.keys(value)[0]; - const nestedMapping = mapping[ - subProperty as keyof typeof mapping - ] as string; - return { clientProperty: nestedMapping, directValue: value[subProperty] }; - } - - return; - } - - /** - * Apply deleted nodes from diff - */ - private applyDeletedNodes(deleted: SnapshotChanges): void { - if (!deleted) return; - for (const nodeId in deleted) { - if (this.mmpService.existNode(nodeId)) { - this.mmpService.removeNode(nodeId, false); - } - } - } - - /** - * Setup handler for map options updates - */ - private setupMapOptionsHandler(): void { - this.socket.on('mapOptionsUpdated', (result: ResponseMapOptionsUpdated) => { - if (result.clientId === this.socket.id) return; - this.mmpService.updateAdditionalMapOptions(result.options); - }); - } - - /** - * Setup handler for map deletion - */ - private setupMapDeletedHandler(): void { - this.socket.on('mapDeleted', () => { - window.location.reload(); - }); - } - - /** - * Setup client-related event handlers - * Handles selection updates and client list changes - */ - private setupClientEventHandlers(): void { - this.setupSelectionHandler(); - this.setupClientListHandler(); - this.setupClientDisconnectHandler(); - } - - /** - * Setup handler for selection updates - */ - private setupSelectionHandler(): void { - this.socket.on('selectionUpdated', (result: ResponseSelectionUpdated) => { - if (result.clientId === this.socket.id) return; - if (!this.mmpService.existNode(result.nodeId)) return; - this.handleSelectionUpdate(result); - }); - } - - /** - * Handle individual selection update - */ - private handleSelectionUpdate(result: ResponseSelectionUpdated): void { - this.ensureClientInMapping(result.clientId); - this.updateClientNodeSelection( - result.clientId, - result.nodeId, - result.selected - ); - const colorForNode: string = this.colorForNode(result.nodeId); - this.mmpService.highlightNode(result.nodeId, colorForNode, false); - } - - /** - * Ensure client exists in color mapping - */ - private ensureClientInMapping(clientId: string): void { - if (!this.colorMapping[clientId]) { - this.colorMapping[clientId] = { color: DEFAULT_COLOR, nodeId: '' }; - this.extractClientListForSubscriber(); - } - } - - /** - * Update client's selected node in mapping - */ - private updateClientNodeSelection( - clientId: string, - nodeId: string, - selected: boolean - ): void { - this.colorMapping[clientId].nodeId = selected ? nodeId : ''; - } - - /** - * Setup handler for client list updates - */ - private setupClientListHandler(): void { - this.socket.on('clientListUpdated', (clients: ServerClientList) => { - this.updateColorMapping(clients); - this.extractClientListForSubscriber(); - }); - } - - /** - * Update color mapping from server client list - */ - private updateColorMapping(clients: ServerClientList): void { - this.colorMapping = Object.keys(clients).reduce( - (acc: ClientColorMapping, key: string) => { - acc[key] = { - nodeId: this.colorMapping[key]?.nodeId || '', - color: key === this.socket.id ? DEFAULT_SELF_COLOR : clients[key], - }; - return acc; + setAttachedNode: (node: ExportNodeProperties | null) => + this.attachedNodeSubject.next(node), + setClientColor: (color: string) => { + this.clientColor = color; }, - {} - ); - } - - /** - * Setup handler for client disconnection - */ - private setupClientDisconnectHandler(): void { - this.socket.on('clientDisconnect', (clientId: string) => { - delete this.colorMapping[clientId]; - this.extractClientListForSubscriber(); - }); - } - - /** - * Setup connection event handlers - */ - private setupConnectionEventHandlers(): void { - this.socket.on('disconnect', () => { - this.setConnectionStatusSubject('disconnected'); - }); - } - - // ─── Socket.io error handling ──────────────────────────────── - - /** - * Validate ServerMap structure at runtime - * Ensures fullMapState has required properties before using - */ - private isValidServerMap(map: unknown): map is ServerMap { - if (!map || typeof map !== 'object') return false; - - const serverMap = map as ServerMap; - - return ( - typeof serverMap.uuid === 'string' && - serverMap.uuid.length > 0 && - Array.isArray(serverMap.data) && - serverMap.data.length > 0 && - typeof serverMap.lastModified === 'string' && - typeof serverMap.createdAt === 'string' && - typeof serverMap.deletedAt === 'string' && - typeof serverMap.deleteAfterDays === 'number' && - typeof serverMap.options === 'object' - ); - } - - /** - * Type guard to validate error response structure at runtime - */ - private isValidErrorResponse( - response: OperationResponse - ): response is ValidationErrorResponse | CriticalErrorResponse { - if (response.success !== false) return false; - - const errorResponse = response as - | ValidationErrorResponse - | CriticalErrorResponse; - - const isBasicStructureValid = - typeof errorResponse.errorType === 'string' && - (errorResponse.errorType === 'validation' || - errorResponse.errorType === 'critical') && - typeof errorResponse.code === 'string' && - errorResponse.code.trim() !== '' && - typeof errorResponse.message === 'string'; - - if (!isBasicStructureValid) return false; - - // Validate fullMapState if present - if (errorResponse.fullMapState) { - return this.isValidServerMap(errorResponse.fullMapState); - } - - return true; - } - - /** - * Simplified handler for all operation responses - * If error with fullMapState, reload from server state - */ - private async handleOperationResponse( - response: OperationResponse, - operationName: string - ): Promise { - // Success - operation confirmed by server - if (response.success) { - return; - } - - // Validate error response structure before processing - if (!this.isValidErrorResponse(response)) { - await this.showMalformedResponseError(); - return; - } - - // Error occurred - reload from fullMapState if available - if (response.fullMapState) { - await this.handleRecoverableError(response, operationName); - } else { - // No fullMapState provided - show critical error - await this.handleCriticalError(response); - } - } - - private async showMalformedResponseError(): Promise { - let message: string; - try { - message = await this.utilsService.translate( - 'TOASTS.ERRORS.MALFORMED_RESPONSE' - ); - } catch { - message = 'Invalid server response. Please try again.'; - } - this.dialogService.openCriticalErrorDialog({ - code: 'MALFORMED_RESPONSE', - message, - }); - } - - private async handleRecoverableError( - response: ValidationErrorResponse | CriticalErrorResponse, - operationName: string - ): Promise { - // Reload entire map from server's authoritative state - this.mmpService.new(response.fullMapState.data, false); - - let message: string; - try { - message = await this.utilsService.translate( - 'TOASTS.ERRORS.OPERATION_FAILED_MAP_RELOADED' - ); - } catch { - message = 'Operation failed - map reloaded'; - } - // Show appropriate error notification - this.toastService.showValidationCorrection(operationName, message); - } - - private async handleCriticalError( - response: ValidationErrorResponse | CriticalErrorResponse - ): Promise { - const userMessage = await this.getUserFriendlyErrorMessage( - response.code || 'SERVER_ERROR', - response.message || 'Unknown error' - ); - - this.dialogService.openCriticalErrorDialog({ - code: response.code || 'SERVER_ERROR', - message: userMessage, - }); - } - - /** - * Convert error code to user-friendly translated message - */ - private async getUserFriendlyErrorMessage( - code: string, - _messageKey: string - ): Promise { - const errorKeyMapping: Record = { - NETWORK_TIMEOUT: 'TOASTS.ERRORS.NETWORK_TIMEOUT', - SERVER_ERROR: 'TOASTS.ERRORS.SERVER_ERROR', - AUTH_FAILED: 'TOASTS.ERRORS.AUTH_FAILED', - MALFORMED_REQUEST: 'TOASTS.ERRORS.MALFORMED_REQUEST', - RATE_LIMIT_EXCEEDED: 'TOASTS.ERRORS.RATE_LIMIT_EXCEEDED', - }; - - const translationKey = - errorKeyMapping[code] || 'TOASTS.ERRORS.UNEXPECTED_ERROR'; - - try { - return await this.utilsService.translate(translationKey); - } catch { - return 'An error occurred. Please try again.'; - } - } - - // ─── Yjs initialization ────────────────────────────────────── - - private initYjs(): void { - const uuid = this.getAttachedMap().cachedMap.uuid; - - if (this.hasActiveYjsConnection(uuid)) { - this.reattachYjsListeners(); - return; - } - - this.resetYjs(); - this.yjsMapId = uuid; - this.yDoc = new Y.Doc(); - this.setupYjsConnection(uuid); - this.setupYjsConnectionStatus(); - this.setupYjsMapDeletionHandler(); - this.createYjsListeners(); - } - - private hasActiveYjsConnection(mapId: string): boolean { - return ( - this.yDoc !== null && this.wsProvider !== null && this.yjsMapId === mapId - ); - } - - private reattachYjsListeners(): void { - this.createYjsListeners(); - if (this.yjsSynced) { - this.loadMapFromYDoc(); - this.setupYjsNodesObserver(); - this.setupYjsMapOptionsObserver(); - this.setupYjsAwareness(); - this.settingsService.setEditMode(this.yjsWritable); - this.setConnectionStatusSubject('connected'); - } - } - - private setupYjsConnection(mapId: string): void { - const wsUrl = this.buildYjsWsUrl(); - this.wsProvider = new WebsocketProvider(wsUrl, mapId, this.yDoc, { - params: { secret: this.modificationSecret }, - maxBackoffTime: 5000, - disableBc: true, - }); - - this.setupYjsWriteAccessListener(); - - this.wsProvider.on('sync', (synced: boolean) => { - if (synced && !this.yjsSynced) { - this.handleFirstYjsSync(); - } - }); - } - - private buildYjsWsUrl(): string { - return buildYjsWsUrl(); - } - - private handleFirstYjsSync(): void { - this.yjsSynced = true; - this.loadMapFromYDoc(); - this.setupYjsNodesObserver(); - this.setupYjsMapOptionsObserver(); - this.setupYjsAwareness(); - this.settingsService.setEditMode(this.yjsWritable); - this.setConnectionStatusSubject('connected'); - } - - private setupYjsConnectionStatus(): void { - this.wsProvider.on( - 'status', - (event: { status: 'connected' | 'disconnected' | 'connecting' }) => { - if (!this.wsProvider) return; - if (event.status === 'connected') { - this.setConnectionStatusSubject('connected'); - } else if (event.status === 'disconnected') { - this.setConnectionStatusSubject('disconnected'); - } - } - ); - } - - private setupYjsMapDeletionHandler(): void { - this.wsProvider.on('connection-close', (event: CloseEvent | null) => { - if (event?.code === WS_CLOSE_MAP_DELETED) { - window.location.reload(); - } - }); - } - - // Listen for the server's write-access message (type 4) to set edit mode - private setupYjsWriteAccessListener(): void { - // Prevent "Unable to compute message" warning in y-websocket - this.wsProvider.messageHandlers[MESSAGE_WRITE_ACCESS] = () => { - // no-op: handled via raw WebSocket listener below - }; - - this.wsProvider.on( - 'status', - (event: { status: 'connected' | 'disconnected' | 'connecting' }) => { - if (event.status !== 'connected') return; - this.attachWriteAccessListener(); - } - ); - } - - private attachWriteAccessListener(): void { - const ws = this.wsProvider?.ws; - if (!ws) return; - ws.addEventListener('message', (event: MessageEvent) => { - this.parseWriteAccessMessage(event); - }); - } - - private parseWriteAccessMessage(event: MessageEvent): void { - const data = new Uint8Array(event.data as ArrayBuffer); - const result = parseWriteAccessBytes(data); - if (result !== null) { - this.yjsWritable = result; - this.settingsService.setEditMode(this.yjsWritable); - } - } - - private resetYjs(): void { - this.unsubscribeYjsListeners(); - this.detachYjsObservers(); - const provider = this.wsProvider; - this.wsProvider = null; - if (provider) { - provider.disconnect(); - provider.destroy(); - } - if (this.yDoc) { - this.yDoc.destroy(); - this.yDoc = null; - } - this.yjsSynced = false; - this.yjsWritable = false; - this.yjsMapId = null; - } - - // ─── Yjs: initial map load ─────────────────────────────────── - - private loadMapFromYDoc(): void { - const nodesMap = this.yDoc.getMap('nodes') as Y.Map>; - const snapshot = this.extractSnapshotFromYDoc(nodesMap); - if (snapshot.length > 0) { - this.mmpService.new(snapshot, false); - } - } - - private extractSnapshotFromYDoc( - nodesMap: Y.Map> - ): ExportNodeProperties[] { - const nodes: ExportNodeProperties[] = []; - nodesMap.forEach((yNode: Y.Map) => { - nodes.push(this.yMapToNodeProps(yNode)); - }); - return sortParentFirst(nodes); - } - - // ─── Yjs: MMP event listeners (MMP → Y.Doc) ───────────────── - - private createYjsListeners(): void { - this.unsubscribeYjsListeners(); - this.setupYjsCreateHandler(); - this.setupYjsSelectionHandlers(); - this.setupYjsNodeUpdateHandler(); - this.setupYjsUndoRedoHandlers(); - this.setupYjsNodeCreateHandler(); - this.setupYjsPasteHandler(); - this.setupYjsNodeRemoveHandler(); - } - - private unsubscribeYjsListeners(): void { - this.yjsSubscriptions.forEach(sub => sub.unsubscribe()); - this.yjsSubscriptions = []; - } - - // Handles map import: clear and repopulate Y.Doc nodes - private setupYjsCreateHandler(): void { - this.yjsSubscriptions.push( - this.mmpService.on('create').subscribe((_result: MapCreateEvent) => { - this.attachedNodeSubject.next(this.mmpService.selectNode()); - this.updateAttachedMap(); - if (this.yjsSynced) { - this.writeImportToYDoc(); - } - }) - ); - } - - private setupYjsSelectionHandlers(): void { - this.yjsSubscriptions.push( - this.mmpService - .on('nodeSelect') - .subscribe((nodeProps: ExportNodeProperties) => { - if (!this.yDoc) return; - this.updateYjsAwarenessSelection(nodeProps.id); - this.attachedNodeSubject.next(nodeProps); - }) - ); - - this.yjsSubscriptions.push( - this.mmpService - .on('nodeDeselect') - .subscribe((nodeProps: ExportNodeProperties) => { - if (!this.yDoc) return; - this.updateYjsAwarenessSelection(null); - this.attachedNodeSubject.next(nodeProps); - }) - ); - } - - private setupYjsNodeUpdateHandler(): void { - this.yjsSubscriptions.push( - this.mmpService.on('nodeUpdate').subscribe((result: NodeUpdateEvent) => { - if (!this.yDoc) return; - this.attachedNodeSubject.next(result.nodeProperties); - this.writeNodeUpdateToYDoc(result); - this.updateAttachedMap(); - }) - ); - } - - 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 - .on('nodeCreate') - .subscribe((newNode: ExportNodeProperties) => { - if (!this.yDoc) return; - this.writeNodeCreateToYDoc(newNode); - this.updateAttachedMap(); - this.mmpService.selectNode(newNode.id); - this.mmpService.editNode(); - }) - ); - } - - private setupYjsPasteHandler(): void { - this.yjsSubscriptions.push( - this.mmpService - .on('nodePaste') - .subscribe((newNodes: ExportNodeProperties[]) => { - if (!this.yDoc) return; - this.writeNodesPasteToYDoc(newNodes); - this.updateAttachedMap(); - }) - ); - } - - private setupYjsNodeRemoveHandler(): void { - this.yjsSubscriptions.push( - this.mmpService - .on('nodeRemove') - .subscribe((removedNode: ExportNodeProperties) => { - if (!this.yDoc) return; - this.writeNodeRemoveFromYDoc(removedNode.id); - this.updateAttachedMap(); - }) - ); - } - - // ─── Yjs: write operations (MMP → Y.Doc) ──────────────────── - - 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); - } - - private writeNodeUpdateToYDoc(event: NodeUpdateEvent): void { - const nodesMap = this.yDoc.getMap('nodes') as Y.Map>; - 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); - } - - private writeNodeRemoveFromYDoc(nodeId: string): void { - const nodesMap = this.yDoc.getMap('nodes') as Y.Map>; - nodesMap.delete(nodeId); - } - - private writeNodesPasteToYDoc(nodes: ExportNodeProperties[]): void { - const nodesMap = this.yDoc.getMap('nodes') as Y.Map>; - this.yDoc.transact(() => { - for (const node of nodes) { - const yNode = new Y.Map(); - this.populateYMapFromNodeProps(yNode, node); - nodesMap.set(node.id, yNode); - } - }); - } - - private writeImportToYDoc(): void { - const snapshot = this.mmpService.exportAsJSON(); - const nodesMap = this.yDoc.getMap('nodes') as Y.Map>; - const sorted = sortParentFirst(snapshot); - - this.yDoc.transact(() => { - this.clearAndRepopulateNodes(nodesMap, sorted); - }); - } - - private clearAndRepopulateNodes( - nodesMap: Y.Map>, - snapshot: ExportNodeProperties[] - ): void { - for (const key of Array.from(nodesMap.keys())) { - nodesMap.delete(key); - } - for (const node of snapshot) { - const yNode = new Y.Map(); - this.populateYMapFromNodeProps(yNode, node); - nodesMap.set(node.id, yNode); - } - } - - 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); - } - - private async deleteMapViaHttp(adminId: string): Promise { - const mapId = this.getAttachedMap().cachedMap.uuid; - await this.httpService.delete( - API_URL.ROOT, - `/maps/${mapId}`, - JSON.stringify({ adminId }) - ); - } - - // ─── Yjs: Y.Doc observers (Y.Doc → MMP) ───────────────────── - - private detachYjsObservers(): void { - if (this.yDoc && this.yjsNodesObserver) { - const nodesMap = this.yDoc.getMap('nodes'); - nodesMap.unobserveDeep(this.yjsNodesObserver); - this.yjsNodesObserver = null; - } - if (this.yDoc && this.yjsOptionsObserver) { - const optionsMap = this.yDoc.getMap('mapOptions'); - optionsMap.unobserve(this.yjsOptionsObserver); - this.yjsOptionsObserver = null; - } - if (this.wsProvider && this.yjsAwarenessHandler) { - this.wsProvider.awareness.off('change', this.yjsAwarenessHandler); - this.yjsAwarenessHandler = null; - } - } - - private setupYjsNodesObserver(): void { - const nodesMap = this.yDoc.getMap('nodes') as Y.Map>; - this.yjsNodesObserver = ( - events: Y.YEvent>>>[], - transaction: Y.Transaction - ) => { - if (transaction.local) return; - for (const event of events) { - this.handleYjsNodeEvent(event, nodesMap); - } + setCanUndo: (v: boolean) => this.canUndoSubject.next(v), + setCanRedo: (v: boolean) => this.canRedoSubject.next(v), + updateAttachedMap: () => this.updateAttachedMap(), + emitClientList: () => this.extractClientListForSubscriber(), }; - nodesMap.observeDeep(this.yjsNodesObserver); - } - - private handleYjsNodeEvent( - event: Y.YEvent>>>, - nodesMap: Y.Map> - ): void { - if (event.target === nodesMap) { - this.handleTopLevelNodeChanges(event, nodesMap); - } else { - this.handleNodePropertyChanges(event); - } - } - - // Handles node add/delete/update from the top-level nodes map - private handleTopLevelNodeChanges( - event: Y.YEvent>>>, - nodesMap: Y.Map> - ): void { - const mapEvent = event as unknown as Y.YMapEvent>; - - if (this.isFullMapReplacement(mapEvent, nodesMap)) { - this.loadMapFromYDoc(); - this.showImportToast(); - return; - } - - mapEvent.keysChanged.forEach(key => { - const change = mapEvent.changes.keys.get(key); - if (!change) return; - - if (change.action === 'add') { - this.applyRemoteNodeAdd(nodesMap.get(key)); - } else if (change.action === 'update') { - this.applyRemoteNodeDelete(key); - this.applyRemoteNodeAdd(nodesMap.get(key)); - } else if (change.action === 'delete') { - this.applyRemoteNodeDelete(key); - } - }); - } - - // Show toast to inform the user that a remote client imported a map - private async showImportToast(): Promise { - const msg = await this.utilsService.translate('TOASTS.MAP_IMPORT_SUCCESS'); - if (msg) this.toastrService.success(msg); - } - - // Detects a full map import by checking if a root node is being added/replaced. - // Normal operations never add root nodes, so this reliably identifies imports. - private isFullMapReplacement( - mapEvent: Y.YMapEvent>, - nodesMap: Y.Map> - ): boolean { - for (const [key, change] of mapEvent.changes.keys) { - if (change.action === 'add' || change.action === 'update') { - const yNode = nodesMap.get(key); - if (yNode?.get('isRoot')) return true; - } - } - return false; - } - - // Handles property changes on individual node Y.Maps - private handleNodePropertyChanges( - event: Y.YEvent>>> - ): void { - const yNode = event.target as unknown as Y.Map; - const nodeId = yNode.get('id') as string; - if (!nodeId || !this.mmpService.existNode(nodeId)) return; - - const mapEvent = event as unknown as Y.YMapEvent; - mapEvent.keysChanged.forEach(key => { - this.applyYDocPropertyToMmp(nodeId, key, yNode.get(key)); - }); - } - - private applyRemoteNodeAdd(yNode: Y.Map): void { - if (!yNode) return; - const nodeProps = this.yMapToNodeProps(yNode); - this.mmpService.addNodesFromServer([nodeProps]); - } - - private applyRemoteNodeDelete(nodeId: string): void { - if (this.mmpService.existNode(nodeId)) { - this.mmpService.removeNode(nodeId, false); - } - } - - // Applies a Y.Doc property change to MMP - private applyYDocPropertyToMmp( - nodeId: string, - yjsKey: string, - value: unknown - ): void { - for (const update of resolveMmpPropertyUpdate(yjsKey, value)) { - this.mmpService.updateNode(update.prop, update.val, false, false, nodeId); - } - } - - private setupYjsMapOptionsObserver(): void { - const optionsMap = this.yDoc.getMap('mapOptions'); - this.yjsOptionsObserver = (_: unknown, transaction: Y.Transaction) => { - if (transaction.local) return; - this.applyRemoteMapOptions(); - }; - optionsMap.observe(this.yjsOptionsObserver); - } - - private applyRemoteMapOptions(): void { - const optionsMap = this.yDoc.getMap('mapOptions'); - const options: CachedMapOptions = { - fontMaxSize: (optionsMap.get('fontMaxSize') as number) ?? 28, - fontMinSize: (optionsMap.get('fontMinSize') as number) ?? 6, - fontIncrement: (optionsMap.get('fontIncrement') as number) ?? 2, - }; - this.mmpService.updateAdditionalMapOptions(options); - } - - // ─── Yjs: Awareness (presence, selection, client list) ─────── - - private setupYjsAwareness(): void { - const awareness = this.wsProvider.awareness; - const color = this.resolveClientColor(awareness); - this.clientColor = color; - - awareness.setLocalStateField('user', { - color, - selectedNodeId: null, - }); - - this.yjsAwarenessHandler = () => { - this.updateFromAwareness(); - }; - awareness.on('change', this.yjsAwarenessHandler); - - // Process awareness states already received before the listener was registered - this.updateFromAwareness(); - } - - // Pick a color that doesn't collide with other clients - private resolveClientColor( - awareness: WebsocketProvider['awareness'] - ): string { - const usedColors = new Set(); - for (const [, state] of awareness.getStates()) { - if (state?.user?.color) usedColors.add(state.user.color); - } - return resolveClientColor(this.clientColor, usedColors); - } - - private updateYjsAwarenessSelection(nodeId: string | null): void { - if (!this.wsProvider) return; - this.wsProvider.awareness.setLocalStateField('user', { - color: this.clientColor, - selectedNodeId: nodeId, - }); - } - - // Rebuild client list and highlights from awareness states - private updateFromAwareness(): void { - const newMapping = this.buildColorMappingFromAwareness(); - const affectedNodes = this.findAffectedNodes(newMapping); - this.colorMapping = newMapping; - this.rehighlightNodes(affectedNodes); - this.extractClientListForSubscriber(); - } - - private buildColorMappingFromAwareness(): ClientColorMapping { - const awareness = this.wsProvider.awareness; - const localClientId = this.yDoc.clientID; - const mapping: ClientColorMapping = {}; - - for (const [clientId, state] of awareness.getStates()) { - if (!state?.user) continue; - const isSelf = clientId === localClientId; - mapping[String(clientId)] = { - color: isSelf ? DEFAULT_SELF_COLOR : state.user.color || DEFAULT_COLOR, - nodeId: state.user.selectedNodeId || '', - }; - } - return mapping; - } - - // Collect node IDs that need re-highlighting - private findAffectedNodes(newMapping: ClientColorMapping): Set { - return findAffectedNodes(this.colorMapping, newMapping); - } - - private rehighlightNodes(nodeIds: Set): void { - for (const nodeId of nodeIds) { - if (!this.mmpService.existNode(nodeId)) continue; - const color = this.colorForNode(nodeId); - this.mmpService.highlightNode(nodeId, color, false); - } - } - - // ─── Y.Doc conversion utilities ────────────────────────────── - - private populateYMapFromNodeProps( - yNode: Y.Map, - nodeProps: ExportNodeProperties - ): void { - populateYMapFromNodeProps(yNode, nodeProps); - } - - private yMapToNodeProps(yNode: Y.Map): ExportNodeProperties { - return yMapToNodeProps(yNode); } // ─── Shared utilities ──────────────────────────────────────── - private setConnectionStatusSubject(value: ConnectionStatus) { - this.connectionStatusSubject.next(value); - } - private colorForNode(nodeId: string): string { const matchingClient = this.clientForNode(nodeId); return matchingClient ? this.colorMapping[matchingClient].color : ''; @@ -1783,9 +281,6 @@ export class MapSyncService implements OnDestroy { ); } - /** - * Store private map data locally for admin access - */ private storePrivateMapData(privateServerMap: PrivateServerMap): void { const serverMap = privateServerMap.map; this.storageService.set(serverMap.uuid, { @@ -1797,9 +292,6 @@ export class MapSyncService implements OnDestroy { }); } - /** - * Setup state for newly created map - */ private setupNewMapState(privateServerMap: PrivateServerMap): void { this.prepareMap(privateServerMap.map); this.settingsService.setEditMode(true); @@ -1807,7 +299,13 @@ export class MapSyncService implements OnDestroy { } private async fetchMapFromServer(id: string): Promise { - const response = await this.httpService.get(API_URL.ROOT, '/maps/' + id); + const secretParam = this.modificationSecret + ? `?secret=${encodeURIComponent(this.modificationSecret)}` + : ''; + const response = await this.httpService.get( + API_URL.ROOT, + '/maps/' + id + secretParam + ); if (!response.ok) return null; const json: ServerMap = await response.json(); return json; @@ -1826,16 +324,10 @@ export class MapSyncService implements OnDestroy { return response.json(); } - /** - * Return the key of the map in the storage - */ private createKey(uuid: string): string { return `map-${uuid}`; } - /** - * Converts server map - */ private convertServerMapToMmp(serverMap: ServerMap): MapProperties { return Object.assign({}, serverMap, { lastModified: Date.parse(serverMap.lastModified), diff --git a/teammapper-frontend/src/app/core/services/map-sync/server-types.ts b/teammapper-frontend/src/app/core/services/map-sync/server-types.ts index 91c8be843..d9742b3a9 100644 --- a/teammapper-frontend/src/app/core/services/map-sync/server-types.ts +++ b/teammapper-frontend/src/app/core/services/map-sync/server-types.ts @@ -124,6 +124,7 @@ interface ServerMap { data: MapSnapshot; options: CachedMapOptions; createdAt: string; + writable?: boolean; } interface PrivateServerMap { diff --git a/teammapper-frontend/src/app/core/services/map-sync/socket-io-sync.service.spec.ts b/teammapper-frontend/src/app/core/services/map-sync/socket-io-sync.service.spec.ts new file mode 100644 index 000000000..28b1962a4 --- /dev/null +++ b/teammapper-frontend/src/app/core/services/map-sync/socket-io-sync.service.spec.ts @@ -0,0 +1,244 @@ +import { Subject } from 'rxjs'; +import { SocketIoSyncService } from './socket-io-sync.service'; +import { MapSyncContext } from './map-sync-context'; +import { MmpService } from '../mmp/mmp.service'; +import { SettingsService } from '../settings/settings.service'; +import { UtilsService } from '../utils/utils.service'; +import { ToastrService } from 'ngx-toastr'; +import { MapSyncErrorHandler } from './map-sync-error-handler'; +import { ExportNodeProperties, MapCreateEvent } from '@mmp/map/types'; +import { MapSnapshot } from '@mmp/map/handlers/history'; + +describe('SocketIoSyncService', () => { + let service: SocketIoSyncService; + let ctx: jest.Mocked; + let mmpService: jest.Mocked; + let settingsService: jest.Mocked; + let utilsService: jest.Mocked; + let toastrService: jest.Mocked; + let errorHandler: jest.Mocked; + + // Subjects to simulate MMP events + let createSubject: Subject; + let nodeUpdateSubject: Subject; + let nodeCreateSubject: Subject; + let nodePasteSubject: Subject; + let nodeRemoveSubject: Subject; + let undoSubject: Subject; + let redoSubject: Subject; + let nodeSelectSubject: Subject; + let nodeDeselectSubject: Subject; + + const mockNode: ExportNodeProperties = { + id: 'node-1', + name: 'Test 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, + }; + + beforeEach(() => { + createSubject = new Subject(); + nodeUpdateSubject = new Subject(); + nodeCreateSubject = new Subject(); + nodePasteSubject = new Subject(); + nodeRemoveSubject = new Subject(); + undoSubject = new Subject(); + redoSubject = new Subject(); + nodeSelectSubject = new Subject(); + nodeDeselectSubject = new Subject(); + + const subjectMap: Record> = { + create: createSubject, + nodeUpdate: nodeUpdateSubject, + nodeCreate: nodeCreateSubject, + nodePaste: nodePasteSubject, + nodeRemove: nodeRemoveSubject, + undo: undoSubject, + redo: redoSubject, + nodeSelect: nodeSelectSubject, + nodeDeselect: nodeDeselectSubject, + }; + + mmpService = { + on: jest.fn((event: string) => subjectMap[event] ?? new Subject()), + selectNode: jest.fn().mockReturnValue(mockNode), + editNode: jest.fn(), + undo: jest.fn(), + redo: jest.fn(), + history: jest.fn().mockReturnValue({ + snapshots: [[], [], []] as MapSnapshot[], + index: 2, + }), + highlightNode: jest.fn(), + exportAsJSON: jest.fn().mockReturnValue([]), + } as unknown as jest.Mocked; + + ctx = { + getAttachedMap: jest.fn().mockReturnValue({ + key: 'map-test', + cachedMap: { uuid: 'test-uuid', data: [] }, + }), + getModificationSecret: jest.fn().mockReturnValue('secret'), + getColorMapping: jest.fn().mockReturnValue({}), + getClientColor: jest.fn().mockReturnValue('#ff0000'), + colorForNode: jest.fn().mockReturnValue(''), + setConnectionStatus: jest.fn(), + setColorMapping: jest.fn(), + setAttachedNode: jest.fn(), + setClientColor: jest.fn(), + setCanUndo: jest.fn(), + setCanRedo: jest.fn(), + updateAttachedMap: jest.fn().mockResolvedValue(undefined), + emitClientList: jest.fn(), + } as jest.Mocked; + + settingsService = { + setEditMode: jest.fn(), + } as unknown as jest.Mocked; + + utilsService = {} as unknown as jest.Mocked; + + toastrService = { + error: jest.fn(), + success: jest.fn(), + warning: jest.fn(), + } as unknown as jest.Mocked; + + errorHandler = { + handleOperationResponse: jest.fn(), + } as unknown as jest.Mocked; + + service = new SocketIoSyncService( + ctx, + mmpService, + settingsService, + utilsService, + toastrService, + errorHandler + ); + + // initMap calls createListeners which sets up all the subscriptions + // We need to mock the socket operations, so we call initMap indirectly + // by invoking the private createListeners through initMap + // But initMap also calls listenServerEvents which needs a socket. + // Let's just call createListeners directly via the public initMap path. + // We need to stub socket first. + + // Since connect() creates the socket and initMap needs it, + // we'll test at the unit level by calling initMap after mocking connect. + }); + + describe('updateCanUndoRedo after node mutations', () => { + beforeEach(() => { + // Inject mock socket before setting up listeners + const mockSocket = { + emit: jest.fn(), + on: jest.fn(), + io: { on: jest.fn() }, + id: 'mock-socket-id', + }; + (service as unknown as { socket: unknown }).socket = mockSocket; + (service as unknown as { createListeners: () => void }).createListeners(); + }); + + it('calls setCanUndo and setCanRedo after create event', () => { + createSubject.next({} as MapCreateEvent); + + expect(ctx.setCanUndo).toHaveBeenCalledWith(true); + expect(ctx.setCanRedo).toHaveBeenCalledWith(false); + }); + + it('calls setCanUndo and setCanRedo after nodeUpdate event', () => { + nodeUpdateSubject.next({ + nodeProperties: mockNode, + changedProperty: 'name', + }); + + expect(ctx.setCanUndo).toHaveBeenCalledWith(true); + expect(ctx.setCanRedo).toHaveBeenCalledWith(false); + }); + + it('calls setCanUndo and setCanRedo after nodeCreate event', () => { + nodeCreateSubject.next(mockNode); + + expect(ctx.setCanUndo).toHaveBeenCalledWith(true); + expect(ctx.setCanRedo).toHaveBeenCalledWith(false); + }); + + it('calls setCanUndo and setCanRedo after nodePaste event', () => { + nodePasteSubject.next([mockNode]); + + expect(ctx.setCanUndo).toHaveBeenCalledWith(true); + expect(ctx.setCanRedo).toHaveBeenCalledWith(false); + }); + + it('calls setCanUndo and setCanRedo after nodeRemove event', () => { + nodeRemoveSubject.next(mockNode); + + expect(ctx.setCanUndo).toHaveBeenCalledWith(true); + expect(ctx.setCanRedo).toHaveBeenCalledWith(false); + }); + }); + + describe('undo and redo', () => { + it('calls mmpService.undo and updates canUndoRedo state', () => { + service.undo(); + + expect(mmpService.undo).toHaveBeenCalled(); + expect(ctx.setCanUndo).toHaveBeenCalled(); + expect(ctx.setCanRedo).toHaveBeenCalled(); + }); + + it('calls mmpService.redo and updates canUndoRedo state', () => { + service.redo(); + + expect(mmpService.redo).toHaveBeenCalled(); + expect(ctx.setCanUndo).toHaveBeenCalled(); + expect(ctx.setCanRedo).toHaveBeenCalled(); + }); + }); + + describe('canUndo/canRedo values from history state', () => { + beforeEach(() => { + const mockSocket = { + emit: jest.fn(), + on: jest.fn(), + io: { on: jest.fn() }, + id: 'mock-socket-id', + }; + (service as unknown as { socket: unknown }).socket = mockSocket; + (service as unknown as { createListeners: () => void }).createListeners(); + }); + + it('sets canUndo=false when history index is at start', () => { + mmpService.history.mockReturnValue({ + snapshots: [[]] as MapSnapshot[], + index: 0, + }); + createSubject.next({} as MapCreateEvent); + + expect(ctx.setCanUndo).toHaveBeenCalledWith(false); + expect(ctx.setCanRedo).toHaveBeenCalledWith(false); + }); + + it('sets canRedo=true when history index is before end', () => { + mmpService.history.mockReturnValue({ + snapshots: [[], [], []] as MapSnapshot[], + index: 1, + }); + createSubject.next({} as MapCreateEvent); + + expect(ctx.setCanUndo).toHaveBeenCalledWith(false); + expect(ctx.setCanRedo).toHaveBeenCalledWith(true); + }); + }); +}); diff --git a/teammapper-frontend/src/app/core/services/map-sync/socket-io-sync.service.ts b/teammapper-frontend/src/app/core/services/map-sync/socket-io-sync.service.ts new file mode 100644 index 000000000..fcb6f5f77 --- /dev/null +++ b/teammapper-frontend/src/app/core/services/map-sync/socket-io-sync.service.ts @@ -0,0 +1,664 @@ +import { io, Socket } from 'socket.io-client'; +import { NodePropertyMapping } from '@mmp/index'; +import { + ExportNodeProperties, + MapCreateEvent, + MapProperties, + NodeUpdateEvent, +} from '@mmp/map/types'; +import { + ResponseMapUpdated, + ResponseUndoRedoChanges, + ResponseMapOptionsUpdated, + ResponseNodesAdded, + ResponseNodeRemoved, + ResponseNodeUpdated, + ResponseSelectionUpdated, + ResponseClientNotification, + OperationResponse, + ReversePropertyMapping, +} from './server-types'; +import { MmpService } from '../mmp/mmp.service'; +import { UtilsService } from '../utils/utils.service'; +import { SettingsService } from '../settings/settings.service'; +import { ToastrService } from 'ngx-toastr'; +import { MapDiff, SnapshotChanges } from '@mmp/map/handlers/history'; +import { CachedMapOptions } from '../../../shared/models/cached-map.model'; +import { + MapSyncContext, + DEFAULT_COLOR, + DEFAULT_SELF_COLOR, +} from './map-sync-context'; +import { MapSyncErrorHandler } from './map-sync-error-handler'; +import { SyncStrategy } from './sync-strategy'; + +type ServerClientList = Record; + +export class SocketIoSyncService implements SyncStrategy { + private socket: Socket; + + constructor( + private ctx: MapSyncContext, + private mmpService: MmpService, + private settingsService: SettingsService, + private utilsService: UtilsService, + private toastrService: ToastrService, + private errorHandler: MapSyncErrorHandler + ) {} + + // ─── Connection ───────────────────────────────────────────── + + connect(): void { + const reconnectOptions = { + reconnection: true, + reconnectionDelay: 1000, + reconnectionDelayMax: 5000, + reconnectionAttempts: 60, + randomizationFactor: 0.5, + }; + + const baseHref = + document.querySelector('base')?.getAttribute('href') ?? '/'; + this.socket = + baseHref !== '/' + ? io('', { + path: `${baseHref}socket.io`, + ...reconnectOptions, + }) + : io({ + ...reconnectOptions, + }); + } + + private detach(): void { + if (this.socket) { + this.socket.removeAllListeners(); + this.leaveMap(); + } + } + + destroy(): void { + this.detach(); + } + + initMap(uuid: string): void { + this.createListeners(); + this.listenServerEvents(uuid); + } + + setWritable(): void { + // No-op: Socket.io strategy handles write access differently + } + + undo(): void { + this.mmpService.undo(); + this.updateCanUndoRedo(); + } + + redo(): void { + this.mmpService.redo(); + this.updateCanUndoRedo(); + } + + updateMapOptions(options?: CachedMapOptions): void { + const cachedMapEntry = this.ctx.getAttachedMap(); + this.socket.emit('updateMapOptions', { + mapId: cachedMapEntry.cachedMap.uuid, + options, + modificationSecret: this.ctx.getModificationSecret(), + }); + } + + async deleteMap(adminId: string): Promise { + const cachedMapEntry = this.ctx.getAttachedMap(); + this.socket.emit('deleteMap', { + adminId, + mapId: cachedMapEntry.cachedMap.uuid, + }); + } + + // ─── MMP event listeners (MMP → Socket.io) ───────────────── + + private createListeners(): void { + this.setupCreateHandler(); + this.setupSelectionHandlers(); + this.setupNodeUpdateHandler(); + this.setupUndoRedoHandlers(); + this.setupNodeCreateHandler(); + this.setupPasteHandler(); + this.setupNodeRemoveHandler(); + } + + private setupCreateHandler(): void { + this.mmpService.on('create').subscribe((_result: MapCreateEvent) => { + this.ctx.setAttachedNode(this.mmpService.selectNode()); + this.ctx.updateAttachedMap(); + this.updateMap(); + this.updateCanUndoRedo(); + }); + } + + private setupSelectionHandlers(): void { + this.mmpService + .on('nodeSelect') + .subscribe((nodeProps: ExportNodeProperties) => { + this.updateNodeSelection(nodeProps.id, true); + this.ctx.setAttachedNode(nodeProps); + }); + + this.mmpService + .on('nodeDeselect') + .subscribe((nodeProps: ExportNodeProperties) => { + this.updateNodeSelection(nodeProps.id, false); + this.ctx.setAttachedNode(nodeProps); + }); + } + + private setupNodeUpdateHandler(): void { + this.mmpService.on('nodeUpdate').subscribe((result: NodeUpdateEvent) => { + this.ctx.setAttachedNode(result.nodeProperties); + this.emitUpdateNode(result); + this.ctx.updateAttachedMap(); + this.updateCanUndoRedo(); + }); + } + + private setupUndoRedoHandlers(): void { + this.mmpService.on('undo').subscribe((diff?: MapDiff) => { + this.ctx.setAttachedNode(this.mmpService.selectNode()); + this.ctx.updateAttachedMap(); + this.emitApplyMapChangesByDiff(diff, 'undo'); + this.updateCanUndoRedo(); + }); + + this.mmpService.on('redo').subscribe((diff?: MapDiff) => { + this.ctx.setAttachedNode(this.mmpService.selectNode()); + this.ctx.updateAttachedMap(); + this.emitApplyMapChangesByDiff(diff, 'redo'); + this.updateCanUndoRedo(); + }); + } + + private updateCanUndoRedo(): void { + if (typeof this.mmpService.history !== 'function') return; + const history = this.mmpService.history(); + if (!history?.snapshots) return; + this.ctx.setCanUndo(history.index > 1); + this.ctx.setCanRedo(history.index < history.snapshots.length - 1); + } + + private setupNodeCreateHandler(): void { + this.mmpService + .on('nodeCreate') + .subscribe((newNode: ExportNodeProperties) => { + this.emitAddNode(newNode); + this.ctx.updateAttachedMap(); + this.mmpService.selectNode(newNode.id); + this.mmpService.editNode(); + this.updateCanUndoRedo(); + }); + } + + private setupPasteHandler(): void { + this.mmpService + .on('nodePaste') + .subscribe((newNodes: ExportNodeProperties[]) => { + this.emitAddNodes(newNodes); + this.ctx.updateAttachedMap(); + this.updateCanUndoRedo(); + }); + } + + private setupNodeRemoveHandler(): void { + this.mmpService + .on('nodeRemove') + .subscribe((removedNode: ExportNodeProperties) => { + this.emitRemoveNode(removedNode); + this.ctx.updateAttachedMap(); + this.updateCanUndoRedo(); + }); + } + + // ─── Socket.io emit methods ───────────────────────────────── + + private joinMap(mmpUuid: string, color: string): Promise { + return new Promise((resolve, reject) => { + this.socket.emit( + 'join', + { mapId: mmpUuid, color }, + (serverMap: MapProperties) => { + if (!serverMap) { + reject('Server Map not available'); + return; + } + resolve(serverMap); + } + ); + }); + } + + private leaveMap(): void { + this.socket.emit('leave'); + } + + private emitAddNode(newNode: ExportNodeProperties): void { + this.socket.emit( + 'addNodes', + { + mapId: this.ctx.getAttachedMap().cachedMap.uuid, + nodes: [newNode], + modificationSecret: this.ctx.getModificationSecret(), + }, + async (response: OperationResponse) => { + await this.handleOperationResponse(response, 'add node'); + } + ); + } + + private emitAddNodes(newNodes: ExportNodeProperties[]): void { + this.socket.emit( + 'addNodes', + { + mapId: this.ctx.getAttachedMap().cachedMap.uuid, + nodes: newNodes, + modificationSecret: this.ctx.getModificationSecret(), + }, + async (response: OperationResponse) => { + await this.handleOperationResponse(response, 'add nodes'); + } + ); + } + + private emitUpdateNode(nodeUpdate: NodeUpdateEvent): void { + this.socket.emit( + 'updateNode', + { + mapId: this.ctx.getAttachedMap().cachedMap.uuid, + node: nodeUpdate.nodeProperties, + updatedProperty: nodeUpdate.changedProperty, + modificationSecret: this.ctx.getModificationSecret(), + }, + async (response: OperationResponse) => { + await this.handleOperationResponse(response, 'update node'); + } + ); + } + + private emitRemoveNode(removedNode: ExportNodeProperties): void { + this.socket.emit( + 'removeNode', + { + mapId: this.ctx.getAttachedMap().cachedMap.uuid, + node: removedNode, + modificationSecret: this.ctx.getModificationSecret(), + }, + async (response: OperationResponse) => { + await this.handleOperationResponse(response, 'remove node'); + } + ); + } + + private emitApplyMapChangesByDiff( + diff: MapDiff, + operationType: 'undo' | 'redo' + ): void { + this.socket.emit( + 'applyMapChangesByDiff', + { + mapId: this.ctx.getAttachedMap().cachedMap.uuid, + diff, + modificationSecret: this.ctx.getModificationSecret(), + }, + async (response: OperationResponse) => { + await this.handleOperationResponse( + response, + `${operationType} operation` + ); + } + ); + } + + private updateMap(): void { + const cachedMapEntry = this.ctx.getAttachedMap(); + this.socket.emit('updateMap', { + mapId: cachedMapEntry.cachedMap.uuid, + map: cachedMapEntry.cachedMap, + modificationSecret: this.ctx.getModificationSecret(), + }); + } + + private updateNodeSelection(id: string, selected: boolean): void { + const mapping = this.ctx.getColorMapping(); + this.ctx.setColorMapping({ + ...mapping, + [this.socket.id]: { + color: DEFAULT_SELF_COLOR, + nodeId: selected ? id : '', + }, + }); + + if (!selected) { + const colorForNode = this.ctx.colorForNode(id); + if (colorForNode !== '') + this.mmpService.highlightNode(id, colorForNode, false); + } + + this.socket.emit('updateNodeSelection', { + mapId: this.ctx.getAttachedMap().cachedMap.uuid, + nodeId: id, + selected, + }); + } + + private checkModificationSecret(): void { + this.socket.emit( + 'checkModificationSecret', + { + mapId: this.ctx.getAttachedMap().cachedMap.uuid, + modificationSecret: this.ctx.getModificationSecret(), + }, + (result: boolean) => this.settingsService.setEditMode(result) + ); + } + + // ─── Server event handlers ────────────────────────────────── + + private listenServerEvents(uuid: string): Promise { + this.checkModificationSecret(); + this.setupReconnectionHandler(uuid); + this.setupNotificationHandlers(); + this.setupNodeEventHandlers(); + this.setupMapEventHandlers(); + this.setupClientEventHandlers(); + this.setupConnectionEventHandlers(); + + return this.joinMap(uuid, this.ctx.getClientColor()); + } + + private setupReconnectionHandler(uuid: string): void { + this.socket.io.on('reconnect', async () => { + const serverMap = await this.joinMap(uuid, this.ctx.getClientColor()); + this.ctx.setConnectionStatus('connected'); + this.mmpService.new(serverMap.data, false); + }); + } + + private setupNotificationHandlers(): void { + this.socket.on( + 'clientNotification', + async (notification: ResponseClientNotification) => { + if (notification.clientId === this.socket.id) return; + await this.handleClientNotification(notification); + } + ); + } + + private async handleClientNotification( + notification: ResponseClientNotification + ): Promise { + const msg = await this.utilsService.translate(notification.message); + if (!msg) return; + + const toastHandlers: Record void> = { + error: () => this.toastrService.error(msg), + success: () => this.toastrService.success(msg), + warning: () => this.toastrService.warning(msg), + }; + toastHandlers[notification.type]?.(); + } + + private setupNodeEventHandlers(): void { + this.setupNodesAddedHandler(); + this.setupNodeUpdatedHandler(); + this.setupNodeRemovedHandler(); + } + + private setupNodesAddedHandler(): void { + this.socket.on('nodesAdded', (result: ResponseNodesAdded) => { + if (result.clientId === this.socket.id) return; + this.mmpService.addNodesFromServer(result.nodes); + }); + } + + private setupNodeUpdatedHandler(): void { + this.socket.on('nodeUpdated', (result: ResponseNodeUpdated) => { + if (result.clientId === this.socket.id) return; + this.handleNodeUpdate(result); + }); + } + + private handleNodeUpdate(result: ResponseNodeUpdated): void { + const newNode = result.node; + const existingNode = this.mmpService.getNode(newNode.id); + const propertyPath = NodePropertyMapping[result.property]; + const changedValue = UtilsService.get(newNode, propertyPath); + + this.mmpService.updateNode( + result.property, + changedValue, + false, + true, + existingNode.id + ); + } + + private setupNodeRemovedHandler(): void { + this.socket.on('nodeRemoved', (result: ResponseNodeRemoved) => { + if (result.clientId === this.socket.id) return; + if (this.mmpService.existNode(result.nodeId)) { + this.mmpService.removeNode(result.nodeId, false); + } + }); + } + + private setupMapEventHandlers(): void { + this.setupMapUpdatedHandler(); + this.setupMapChangesHandler(); + this.setupMapOptionsHandler(); + this.setupMapDeletedHandler(); + } + + private setupMapUpdatedHandler(): void { + this.socket.on('mapUpdated', (result: ResponseMapUpdated) => { + if (result.clientId === this.socket.id) return; + this.mmpService.new(result.map.data, false); + }); + } + + private setupMapChangesHandler(): void { + this.socket.on('mapChangesUndoRedo', (result: ResponseUndoRedoChanges) => { + if (result.clientId === this.socket.id) return; + this.applyMapDiff(result.diff); + }); + } + + private applyMapDiff(diff: MapDiff): void { + this.applyAddedNodes(diff.added); + this.applyUpdatedNodes(diff.updated); + this.applyDeletedNodes(diff.deleted); + } + + private applyAddedNodes(added: SnapshotChanges): void { + if (!added) return; + for (const nodeId in added) { + this.mmpService.addNode(added[nodeId], false); + } + } + + private applyUpdatedNodes(updated: SnapshotChanges): void { + if (!updated) return; + for (const nodeId in updated) { + if (this.mmpService.existNode(nodeId)) { + this.applyNodePropertyUpdates(nodeId, updated[nodeId]); + } + } + } + + private applyNodePropertyUpdates( + nodeId: string, + nodeUpdates: Partial + ): void { + if (!nodeUpdates) return; + + for (const property in nodeUpdates) { + const updatedProperty = this.getClientProperty( + property, + (nodeUpdates as Record)[property] + ); + if (updatedProperty) { + this.mmpService.updateNode( + updatedProperty.clientProperty, + updatedProperty.directValue, + false, + true, + nodeId + ); + } + } + } + + private getClientProperty( + serverProperty: string, + value: unknown + ): { clientProperty: string; directValue: unknown } | undefined { + const mapping = + ReversePropertyMapping[ + serverProperty as keyof typeof ReversePropertyMapping + ]; + + if (typeof mapping === 'string') { + return { clientProperty: mapping, directValue: value }; + } + + if (mapping && typeof value === 'object') { + const subProperty = Object.keys(value)[0]; + const nestedMapping = mapping[ + subProperty as keyof typeof mapping + ] as string; + return { clientProperty: nestedMapping, directValue: value[subProperty] }; + } + + return; + } + + private applyDeletedNodes(deleted: SnapshotChanges): void { + if (!deleted) return; + for (const nodeId in deleted) { + if (this.mmpService.existNode(nodeId)) { + this.mmpService.removeNode(nodeId, false); + } + } + } + + private setupMapOptionsHandler(): void { + this.socket.on('mapOptionsUpdated', (result: ResponseMapOptionsUpdated) => { + if (result.clientId === this.socket.id) return; + this.mmpService.updateAdditionalMapOptions(result.options); + }); + } + + private setupMapDeletedHandler(): void { + this.socket.on('mapDeleted', () => { + window.location.reload(); + }); + } + + private setupClientEventHandlers(): void { + this.setupSelectionHandler(); + this.setupClientListHandler(); + this.setupClientDisconnectHandler(); + } + + private setupSelectionHandler(): void { + this.socket.on('selectionUpdated', (result: ResponseSelectionUpdated) => { + if (result.clientId === this.socket.id) return; + if (!this.mmpService.existNode(result.nodeId)) return; + this.handleSelectionUpdate(result); + }); + } + + private handleSelectionUpdate(result: ResponseSelectionUpdated): void { + this.ensureClientInMapping(result.clientId); + this.updateClientNodeSelection( + result.clientId, + result.nodeId, + result.selected + ); + const colorForNode = this.ctx.colorForNode(result.nodeId); + this.mmpService.highlightNode(result.nodeId, colorForNode, false); + } + + private ensureClientInMapping(clientId: string): void { + const mapping = this.ctx.getColorMapping(); + if (!mapping[clientId]) { + this.ctx.setColorMapping({ + ...mapping, + [clientId]: { color: DEFAULT_COLOR, nodeId: '' }, + }); + this.ctx.emitClientList(); + } + } + + private updateClientNodeSelection( + clientId: string, + nodeId: string, + selected: boolean + ): void { + const mapping = this.ctx.getColorMapping(); + const client = mapping[clientId]; + if (!client) return; + this.ctx.setColorMapping({ + ...mapping, + [clientId]: { ...client, nodeId: selected ? nodeId : '' }, + }); + } + + private setupClientListHandler(): void { + this.socket.on('clientListUpdated', (clients: ServerClientList) => { + this.updateColorMapping(clients); + this.ctx.emitClientList(); + }); + } + + private updateColorMapping(clients: ServerClientList): void { + const currentMapping = this.ctx.getColorMapping(); + this.ctx.setColorMapping( + Object.fromEntries( + Object.keys(clients).map(key => [ + key, + { + nodeId: currentMapping[key]?.nodeId || '', + color: key === this.socket.id ? DEFAULT_SELF_COLOR : clients[key], + }, + ]) + ) + ); + } + + private setupClientDisconnectHandler(): void { + this.socket.on('clientDisconnect', (clientId: string) => { + const mapping = this.ctx.getColorMapping(); + this.ctx.setColorMapping( + Object.fromEntries( + Object.entries(mapping).filter(([key]) => key !== clientId) + ) + ); + this.ctx.emitClientList(); + }); + } + + private setupConnectionEventHandlers(): void { + this.socket.on('disconnect', () => { + this.ctx.setConnectionStatus('disconnected'); + }); + } + + // ─── Error handling (delegated) ───────────────────────────── + + private async handleOperationResponse( + response: OperationResponse, + operationName: string + ): Promise { + return this.errorHandler.handleOperationResponse(response, operationName); + } +} diff --git a/teammapper-frontend/src/app/core/services/map-sync/sync-strategy.ts b/teammapper-frontend/src/app/core/services/map-sync/sync-strategy.ts new file mode 100644 index 000000000..4d7b4dced --- /dev/null +++ b/teammapper-frontend/src/app/core/services/map-sync/sync-strategy.ts @@ -0,0 +1,31 @@ +import { CachedMapOptions } from '../../../shared/models/cached-map.model'; + +export interface SyncStrategy { + /** One-time connection setup. Socket.io: opens socket. Yjs: no-op. */ + connect(): void; + + /** + * Initialize syncing for the given map. + * Caller must call destroy() before initMap() when switching maps. + * Caller must call setWritable() before initMap() to set permissions. + */ + initMap(uuid: string): void; + + /** Full cleanup: destroy resources, disconnect, reset all state. Idempotent. */ + destroy(): void; + + /** Undo the last operation. */ + undo(): void; + + /** Redo the last undone operation. */ + redo(): void; + + /** Push map option changes to the server. */ + updateMapOptions(options?: CachedMapOptions): void; + + /** Delete the map by admin ID. */ + deleteMap(adminId: string): Promise; + + /** Set write-access status from HTTP response. */ + setWritable(writable: boolean): void; +} diff --git a/teammapper-frontend/src/app/core/services/map-sync/yjs-sync.service.spec.ts b/teammapper-frontend/src/app/core/services/map-sync/yjs-sync.service.spec.ts new file mode 100644 index 000000000..39e80f2ff --- /dev/null +++ b/teammapper-frontend/src/app/core/services/map-sync/yjs-sync.service.spec.ts @@ -0,0 +1,114 @@ +import { MmpService } from '../mmp/mmp.service'; +import { SettingsService } from '../settings/settings.service'; +import { UtilsService } from '../utils/utils.service'; +import { ToastrService } from 'ngx-toastr'; +import { HttpService } from '../../http/http.service'; +import { MapSyncContext } from './map-sync-context'; +import { YjsSyncService } from './yjs-sync.service'; + +function createMockContext(): MapSyncContext { + return { + getAttachedMap: jest.fn().mockReturnValue({ + key: 'map-test', + cachedMap: { uuid: 'test-uuid', data: [] }, + }), + getModificationSecret: jest.fn().mockReturnValue('secret'), + getColorMapping: jest.fn().mockReturnValue({}), + getClientColor: jest.fn().mockReturnValue('#ff0000'), + colorForNode: jest.fn().mockReturnValue(''), + setConnectionStatus: jest.fn(), + setColorMapping: jest.fn(), + setAttachedNode: jest.fn(), + setClientColor: jest.fn(), + setCanUndo: jest.fn(), + setCanRedo: jest.fn(), + updateAttachedMap: jest.fn(), + emitClientList: jest.fn(), + }; +} + +function createMockMmpService(): jest.Mocked { + return { + on: jest.fn().mockReturnValue({ + subscribe: jest.fn().mockReturnValue({ unsubscribe: jest.fn() }), + }), + selectNode: jest.fn(), + existNode: jest.fn().mockReturnValue(true), + exportAsJSON: jest.fn().mockReturnValue([]), + } as unknown as jest.Mocked; +} + +function createService(): YjsSyncService { + return new YjsSyncService( + createMockContext(), + createMockMmpService(), + {} as SettingsService, + {} as UtilsService, + {} as ToastrService, + {} as HttpService + ); +} + +interface YjsSyncInternals { + yjsWritable: boolean; + yDoc: unknown; +} + +function internals(service: YjsSyncService): YjsSyncInternals { + return service as unknown as YjsSyncInternals; +} + +describe('YjsSyncService', () => { + describe('setWritable', () => { + let service: YjsSyncService; + + beforeEach(() => { + service = createService(); + }); + + it('sets yjsWritable to true', () => { + service.setWritable(true); + + expect(internals(service).yjsWritable).toBe(true); + }); + + it('sets yjsWritable to false', () => { + service.setWritable(false); + + expect(internals(service).yjsWritable).toBe(false); + }); + }); + + describe('initMap does not alter writable state', () => { + let service: YjsSyncService; + + beforeEach(() => { + service = createService(); + }); + + afterEach(() => { + service.destroy(); + }); + + it('does not reset yjsWritable when called', () => { + service.setWritable(true); + service.initMap('test-uuid'); + + expect(internals(service).yjsWritable).toBe(true); + }); + + it('creates a new yDoc', () => { + service.initMap('test-uuid'); + + expect(internals(service).yDoc).not.toBeNull(); + }); + + it('retains writable after destroy-setWritable-initMap sequence', () => { + service.destroy(); + service.setWritable(true); + service.initMap('test-uuid'); + + expect(internals(service).yjsWritable).toBe(true); + }); + }); +}); diff --git a/teammapper-frontend/src/app/core/services/map-sync/yjs-sync.service.ts b/teammapper-frontend/src/app/core/services/map-sync/yjs-sync.service.ts new file mode 100644 index 000000000..83b4e4921 --- /dev/null +++ b/teammapper-frontend/src/app/core/services/map-sync/yjs-sync.service.ts @@ -0,0 +1,650 @@ +import { Subscription } from 'rxjs'; +import { NodePropertyMapping } from '@mmp/index'; +import { + ExportNodeProperties, + MapCreateEvent, + NodeUpdateEvent, +} from '@mmp/map/types'; +import * as Y from 'yjs'; +import { WebsocketProvider } from 'y-websocket'; +import { MmpService } from '../mmp/mmp.service'; +import { SettingsService } from '../settings/settings.service'; +import { UtilsService } from '../utils/utils.service'; +import { ToastrService } from 'ngx-toastr'; +import { API_URL, HttpService } from '../../http/http.service'; +import { CachedMapOptions } from '../../../shared/models/cached-map.model'; +import { + ClientColorMapping, + populateYMapFromNodeProps, + yMapToNodeProps, + buildYjsWsUrl, + resolveClientColor, + findAffectedNodes, + resolveMmpPropertyUpdate, + sortParentFirst, + collectDescendantIds, +} from './yjs-utils'; +import { + MapSyncContext, + DEFAULT_COLOR, + DEFAULT_SELF_COLOR, +} from './map-sync-context'; +import { SyncStrategy } from './sync-strategy'; + +const WS_CLOSE_MAP_DELETED = 4001; + +export class YjsSyncService implements SyncStrategy { + private yDoc: Y.Doc | null = null; + private wsProvider: WebsocketProvider | null = null; + private yjsSynced = false; + private yjsWritable = false; + private yjsSubscriptions: Subscription[] = []; + private yjsMapId: string | null = null; + private yjsNodesObserver: + | Parameters['observeDeep']>[0] + | null = null; + private yjsOptionsObserver: Parameters['observe']>[0] | null = + null; + private yjsAwarenessHandler: (() => void) | null = null; + private yUndoManager: Y.UndoManager | null = null; + + constructor( + private ctx: MapSyncContext, + private mmpService: MmpService, + private settingsService: SettingsService, + private utilsService: UtilsService, + private toastrService: ToastrService, + private httpService: HttpService + ) {} + + // ─── Public API ───────────────────────────────────────────── + + setWritable(writable: boolean): void { + this.yjsWritable = writable; + } + + undo(): void { + this.yUndoManager?.undo(); + } + + redo(): void { + this.yUndoManager?.redo(); + } + + updateMapOptions(options?: CachedMapOptions): void { + this.writeMapOptionsToYDoc(options); + } + + async deleteMap(adminId: string): Promise { + await this.deleteMapViaHttp(adminId); + } + + // ─── SyncStrategy lifecycle ───────────────────────────────── + + connect(): void { + // Yjs connects lazily in initMap — no-op here + } + + initMap(uuid: string): void { + if (this.hasActiveConnection(uuid)) { + this.reattachListeners(); + return; + } + + this.yjsMapId = uuid; + this.yDoc = new Y.Doc(); + this.setupConnection(uuid); + this.setupConnectionStatus(); + this.setupMapDeletionHandler(); + this.createListeners(); + } + + private hasActiveConnection(mapId: string): boolean { + return ( + this.yDoc !== null && this.wsProvider !== null && this.yjsMapId === mapId + ); + } + + private reattachListeners(): void { + this.createListeners(); + if (this.yjsSynced) { + this.loadMapFromYDoc(); + this.setupNodesObserver(); + this.setupMapOptionsObserver(); + this.setupAwareness(); + this.settingsService.setEditMode(this.yjsWritable); + this.ctx.setConnectionStatus('connected'); + } + } + + private setupConnection(mapId: string): void { + const wsUrl = buildYjsWsUrl(); + this.wsProvider = new WebsocketProvider(wsUrl, mapId, this.yDoc, { + params: { secret: this.ctx.getModificationSecret() }, + maxBackoffTime: 5000, + disableBc: true, + }); + + this.wsProvider.on('sync', (synced: boolean) => { + if (synced && !this.yjsSynced) { + this.handleFirstSync(); + } + }); + } + + private handleFirstSync(): void { + this.yjsSynced = true; + this.loadMapFromYDoc(); + this.setupNodesObserver(); + this.setupMapOptionsObserver(); + this.initUndoManager(); + this.setupAwareness(); + this.settingsService.setEditMode(this.yjsWritable); + this.ctx.setConnectionStatus('connected'); + } + + private initUndoManager(): 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.ctx.setCanUndo(this.yUndoManager.undoStack.length > 0); + this.ctx.setCanRedo(this.yUndoManager.redoStack.length > 0); + }; + + this.yUndoManager.on('stack-item-added', updateUndoRedoState); + this.yUndoManager.on('stack-item-popped', updateUndoRedoState); + } + + private setupConnectionStatus(): void { + this.wsProvider.on( + 'status', + (event: { status: 'connected' | 'disconnected' | 'connecting' }) => { + if (!this.wsProvider) return; + if (event.status === 'connected') { + this.ctx.setConnectionStatus('connected'); + } else if (event.status === 'disconnected') { + this.ctx.setConnectionStatus('disconnected'); + } + } + ); + } + + private setupMapDeletionHandler(): void { + this.wsProvider.on('connection-close', (event: CloseEvent | null) => { + if (event?.code === WS_CLOSE_MAP_DELETED) { + window.location.reload(); + } + }); + } + + // ─── Cleanup ──────────────────────────────────────────────── + + destroy(): void { + this.unsubscribeListeners(); + this.detachObservers(); + if (this.yUndoManager) { + this.yUndoManager.destroy(); + this.yUndoManager = null; + this.ctx.setCanUndo(false); + this.ctx.setCanRedo(false); + } + const provider = this.wsProvider; + this.wsProvider = null; + if (provider) { + provider.disconnect(); + provider.destroy(); + } + if (this.yDoc) { + this.yDoc.destroy(); + this.yDoc = null; + } + this.yjsSynced = false; + this.yjsWritable = false; + this.yjsMapId = null; + } + + private detachObservers(): void { + if (this.yDoc && this.yjsNodesObserver) { + const nodesMap = this.yDoc.getMap('nodes'); + nodesMap.unobserveDeep(this.yjsNodesObserver); + this.yjsNodesObserver = null; + } + if (this.yDoc && this.yjsOptionsObserver) { + const optionsMap = this.yDoc.getMap('mapOptions'); + optionsMap.unobserve(this.yjsOptionsObserver); + this.yjsOptionsObserver = null; + } + if (this.wsProvider && this.yjsAwarenessHandler) { + this.wsProvider.awareness.off('change', this.yjsAwarenessHandler); + this.yjsAwarenessHandler = null; + } + } + + private unsubscribeListeners(): void { + this.yjsSubscriptions.forEach(sub => sub.unsubscribe()); + this.yjsSubscriptions = []; + } + + // ─── Initial map load ─────────────────────────────────────── + + private loadMapFromYDoc(): void { + const nodesMap = this.yDoc.getMap('nodes') as Y.Map>; + const snapshot = this.extractSnapshotFromYDoc(nodesMap); + if (snapshot.length > 0) { + this.mmpService.new(snapshot, false); + } + } + + private extractSnapshotFromYDoc( + nodesMap: Y.Map> + ): ExportNodeProperties[] { + const nodes: ExportNodeProperties[] = []; + nodesMap.forEach((yNode: Y.Map) => { + nodes.push(yMapToNodeProps(yNode)); + }); + return sortParentFirst(nodes); + } + + // ─── MMP event listeners (MMP → Y.Doc) ───────────────────── + + private createListeners(): void { + this.unsubscribeListeners(); + this.setupCreateHandler(); + this.setupSelectionHandlers(); + this.setupNodeUpdateHandler(); + this.setupNodeCreateHandler(); + this.setupPasteHandler(); + this.setupNodeRemoveHandler(); + } + + private setupCreateHandler(): void { + this.yjsSubscriptions.push( + this.mmpService.on('create').subscribe((_result: MapCreateEvent) => { + this.ctx.setAttachedNode(this.mmpService.selectNode()); + this.ctx.updateAttachedMap(); + if (this.yjsSynced) { + this.writeImportToYDoc(); + } + }) + ); + } + + private setupSelectionHandlers(): void { + this.yjsSubscriptions.push( + this.mmpService + .on('nodeSelect') + .subscribe((nodeProps: ExportNodeProperties) => { + if (!this.yDoc) return; + this.updateAwarenessSelection(nodeProps.id); + this.ctx.setAttachedNode(nodeProps); + }) + ); + + this.yjsSubscriptions.push( + this.mmpService + .on('nodeDeselect') + .subscribe((nodeProps: ExportNodeProperties) => { + if (!this.yDoc) return; + this.updateAwarenessSelection(null); + this.ctx.setAttachedNode(nodeProps); + }) + ); + } + + private setupNodeUpdateHandler(): void { + this.yjsSubscriptions.push( + this.mmpService.on('nodeUpdate').subscribe((result: NodeUpdateEvent) => { + if (!this.yDoc) return; + this.ctx.setAttachedNode(result.nodeProperties); + this.writeNodeUpdateToYDoc(result); + this.ctx.updateAttachedMap(); + }) + ); + } + + private setupNodeCreateHandler(): void { + this.yjsSubscriptions.push( + this.mmpService + .on('nodeCreate') + .subscribe((newNode: ExportNodeProperties) => { + if (!this.yDoc) return; + this.writeNodeCreateToYDoc(newNode); + this.ctx.updateAttachedMap(); + this.mmpService.selectNode(newNode.id); + this.mmpService.editNode(); + }) + ); + } + + private setupPasteHandler(): void { + this.yjsSubscriptions.push( + this.mmpService + .on('nodePaste') + .subscribe((newNodes: ExportNodeProperties[]) => { + if (!this.yDoc) return; + this.writeNodesPasteToYDoc(newNodes); + this.ctx.updateAttachedMap(); + }) + ); + } + + private setupNodeRemoveHandler(): void { + this.yjsSubscriptions.push( + this.mmpService + .on('nodeRemove') + .subscribe((removedNode: ExportNodeProperties) => { + if (!this.yDoc) return; + this.writeNodeRemoveFromYDoc(removedNode.id); + this.ctx.updateAttachedMap(); + }) + ); + } + + // ─── Write operations (MMP → Y.Doc) ──────────────────────── + + private writeNodeCreateToYDoc(nodeProps: ExportNodeProperties): void { + const nodesMap = this.yDoc.getMap('nodes') as Y.Map>; + this.yDoc.transact(() => { + const yNode = new Y.Map(); + populateYMapFromNodeProps(yNode, nodeProps); + nodesMap.set(nodeProps.id, yNode); + }, 'local'); + } + + private writeNodeUpdateToYDoc(event: NodeUpdateEvent): void { + const nodesMap = this.yDoc.getMap('nodes') as Y.Map>; + const yNode = nodesMap.get(event.nodeProperties.id); + if (!yNode) return; + + 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 { + const nodesMap = this.yDoc.getMap('nodes') as Y.Map>; + if (!nodesMap.has(nodeId)) return; + + const descendantIds = collectDescendantIds(nodesMap, nodeId); + + this.yDoc.transact(() => { + nodesMap.delete(nodeId); + for (const id of descendantIds) { + nodesMap.delete(id); + } + }, 'local'); + } + + private writeNodesPasteToYDoc(nodes: ExportNodeProperties[]): void { + const nodesMap = this.yDoc.getMap('nodes') as Y.Map>; + this.yDoc.transact(() => { + for (const node of nodes) { + const yNode = new Y.Map(); + populateYMapFromNodeProps(yNode, node); + nodesMap.set(node.id, yNode); + } + }, 'local'); + } + + private writeImportToYDoc(): void { + const snapshot = this.mmpService.exportAsJSON(); + const nodesMap = this.yDoc.getMap('nodes') as Y.Map>; + const sorted = sortParentFirst(snapshot); + + this.yDoc.transact(() => { + this.clearAndRepopulateNodes(nodesMap, sorted); + }, 'import'); + } + + private clearAndRepopulateNodes( + nodesMap: Y.Map>, + snapshot: ExportNodeProperties[] + ): void { + for (const key of Array.from(nodesMap.keys())) { + nodesMap.delete(key); + } + for (const node of snapshot) { + const yNode = new Y.Map(); + populateYMapFromNodeProps(yNode, node); + nodesMap.set(node.id, yNode); + } + } + + private writeMapOptionsToYDoc(options?: CachedMapOptions): void { + if (!this.yDoc || !options) return; + const optionsMap = this.yDoc.getMap('mapOptions'); + 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 { + const mapId = this.ctx.getAttachedMap().cachedMap.uuid; + await this.httpService.delete( + API_URL.ROOT, + `/maps/${mapId}`, + JSON.stringify({ adminId }) + ); + } + + // ─── Y.Doc observers (Y.Doc → MMP) ───────────────────────── + + private setupNodesObserver(): void { + const nodesMap = this.yDoc.getMap('nodes') as Y.Map>; + this.yjsNodesObserver = ( + events: Y.YEvent>>>[], + transaction: Y.Transaction + ) => { + if (transaction.local && transaction.origin !== this.yUndoManager) return; + for (const event of events) { + this.handleNodeEvent(event, nodesMap); + } + }; + nodesMap.observeDeep(this.yjsNodesObserver); + } + + private handleNodeEvent( + event: Y.YEvent>>>, + nodesMap: Y.Map> + ): void { + if (event.target === nodesMap) { + this.handleTopLevelNodeChanges(event, nodesMap); + } else { + this.handleNodePropertyChanges(event); + } + } + + private handleTopLevelNodeChanges( + event: Y.YEvent>>>, + nodesMap: Y.Map> + ): void { + const mapEvent = event as unknown as Y.YMapEvent>; + + if (this.isFullMapReplacement(mapEvent, nodesMap)) { + this.loadMapFromYDoc(); + this.showImportToast(); + return; + } + + const adds: string[] = []; + + mapEvent.keysChanged.forEach(key => { + const change = mapEvent.changes.keys.get(key); + if (!change) return; + + if (change.action === 'add') { + adds.push(key); + } else if (change.action === 'update') { + this.applyRemoteNodeDelete(key); + this.applyRemoteNodeAdd(nodesMap.get(key)); + } else if (change.action === 'delete') { + this.applyRemoteNodeDelete(key); + } + }); + + if (adds.length > 0) { + const nodeProps = adds + .map(key => nodesMap.get(key)) + .filter((yNode): yNode is Y.Map => !!yNode) + .map(yNode => yMapToNodeProps(yNode)); + const sorted = sortParentFirst(nodeProps); + sorted.forEach(props => this.mmpService.addNodesFromServer([props])); + } + } + + private async showImportToast(): Promise { + const msg = await this.utilsService.translate('TOASTS.MAP_IMPORT_SUCCESS'); + if (msg) this.toastrService.success(msg); + } + + private isFullMapReplacement( + mapEvent: Y.YMapEvent>, + nodesMap: Y.Map> + ): boolean { + for (const [key, change] of mapEvent.changes.keys) { + if (change.action === 'add' || change.action === 'update') { + const yNode = nodesMap.get(key); + if (yNode?.get('isRoot')) return true; + } + } + return false; + } + + private handleNodePropertyChanges( + event: Y.YEvent>>> + ): void { + const yNode = event.target as unknown as Y.Map; + const nodeId = yNode.get('id') as string; + if (!nodeId || !this.mmpService.existNode(nodeId)) return; + + const mapEvent = event as unknown as Y.YMapEvent; + mapEvent.keysChanged.forEach(key => { + this.applyYDocPropertyToMmp(nodeId, key, yNode.get(key)); + }); + } + + private applyRemoteNodeAdd(yNode: Y.Map): void { + if (!yNode) return; + const nodeProps = yMapToNodeProps(yNode); + this.mmpService.addNodesFromServer([nodeProps]); + } + + private applyRemoteNodeDelete(nodeId: string): void { + if (this.mmpService.existNode(nodeId)) { + this.mmpService.removeNode(nodeId, false); + } + } + + private applyYDocPropertyToMmp( + nodeId: string, + yjsKey: string, + value: unknown + ): void { + for (const update of resolveMmpPropertyUpdate(yjsKey, value)) { + this.mmpService.updateNode(update.prop, update.val, false, false, nodeId); + } + } + + private setupMapOptionsObserver(): void { + const optionsMap = this.yDoc.getMap('mapOptions'); + this.yjsOptionsObserver = (_: unknown, transaction: Y.Transaction) => { + if (transaction.local && transaction.origin !== this.yUndoManager) return; + this.applyRemoteMapOptions(); + }; + optionsMap.observe(this.yjsOptionsObserver); + } + + private applyRemoteMapOptions(): void { + const optionsMap = this.yDoc.getMap('mapOptions'); + const options: CachedMapOptions = { + fontMaxSize: (optionsMap.get('fontMaxSize') as number) ?? 28, + fontMinSize: (optionsMap.get('fontMinSize') as number) ?? 6, + fontIncrement: (optionsMap.get('fontIncrement') as number) ?? 2, + }; + this.mmpService.updateAdditionalMapOptions(options); + } + + // ─── Awareness (presence, selection, client list) ─────────── + + private setupAwareness(): void { + const awareness = this.wsProvider.awareness; + const color = this.pickClientColor(awareness); + this.ctx.setClientColor(color); + + awareness.setLocalStateField('user', { + color, + selectedNodeId: null, + }); + + this.yjsAwarenessHandler = () => { + this.updateFromAwareness(); + }; + awareness.on('change', this.yjsAwarenessHandler); + + // Process awareness states already received before the listener + this.updateFromAwareness(); + } + + private pickClientColor(awareness: WebsocketProvider['awareness']): string { + const usedColors = new Set(); + for (const [, state] of awareness.getStates()) { + if (state?.user?.color) usedColors.add(state.user.color); + } + return resolveClientColor(this.ctx.getClientColor(), usedColors); + } + + private updateAwarenessSelection(nodeId: string | null): void { + if (!this.wsProvider) return; + this.wsProvider.awareness.setLocalStateField('user', { + color: this.ctx.getClientColor(), + selectedNodeId: nodeId, + }); + } + + private updateFromAwareness(): void { + const newMapping = this.buildColorMappingFromAwareness(); + const affectedNodes = findAffectedNodes( + this.ctx.getColorMapping(), + newMapping + ); + this.ctx.setColorMapping(newMapping); + this.rehighlightNodes(affectedNodes); + this.ctx.emitClientList(); + } + + private buildColorMappingFromAwareness(): ClientColorMapping { + const awareness = this.wsProvider.awareness; + const localClientId = this.yDoc.clientID; + const mapping: ClientColorMapping = {}; + + for (const [clientId, state] of awareness.getStates()) { + if (!state?.user) continue; + const isSelf = clientId === localClientId; + mapping[String(clientId)] = { + color: isSelf ? DEFAULT_SELF_COLOR : state.user.color || DEFAULT_COLOR, + nodeId: state.user.selectedNodeId || '', + }; + } + return mapping; + } + + private rehighlightNodes(nodeIds: Set): void { + for (const nodeId of nodeIds) { + if (!this.mmpService.existNode(nodeId)) continue; + const color = this.ctx.colorForNode(nodeId); + this.mmpService.highlightNode(nodeId, color, false); + } + } +} 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 000000000..4d9d64807 --- /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/map-sync/yjs-utils.spec.ts b/teammapper-frontend/src/app/core/services/map-sync/yjs-utils.spec.ts new file mode 100644 index 000000000..ba5ded535 --- /dev/null +++ b/teammapper-frontend/src/app/core/services/map-sync/yjs-utils.spec.ts @@ -0,0 +1,544 @@ +import * as Y from 'yjs'; +import { ExportNodeProperties } from '@mmp/map/types'; +import { + populateYMapFromNodeProps, + yMapToNodeProps, + buildYjsWsUrl, + resolveClientColor, + findAffectedNodes, + resolveMmpPropertyUpdate, + resolveCompoundMmpUpdates, + sortParentFirst, + collectDescendantIds, +} from './yjs-utils'; + +// Mock the NodePropertyMapping module +jest.mock('@mmp/index', () => ({ + NodePropertyMapping: { + name: ['name'], + locked: ['locked'], + coordinates: ['coordinates'], + imageSrc: ['image', 'src'], + imageSize: ['image', 'size'], + linkHref: ['link', 'href'], + backgroundColor: ['colors', 'background'], + branchColor: ['colors', 'branch'], + fontWeight: ['font', 'weight'], + fontStyle: ['font', 'style'], + fontSize: ['font', 'size'], + nameColor: ['colors', 'name'], + hidden: ['hidden'], + }, +})); + +// Import NodePropertyMapping after mocking - needed for reverse mapping +// eslint-disable-next-line @typescript-eslint/no-unused-vars +import { NodePropertyMapping } from '@mmp/index'; + +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, + }; +} + +// ─── Y.Doc conversion utilities ────────────────────────────── + +describe('Y.Doc conversion utilities', () => { + let doc: Y.Doc; + let nodesMap: Y.Map>; + + beforeEach(() => { + doc = new Y.Doc(); + nodesMap = doc.getMap('nodes') as Y.Map>; + }); + + afterEach(() => { + doc.destroy(); + }); + + it('round-trips node properties through Y.Map', () => { + const input = createMockNode({ + id: 'n1', + name: 'Hello', + parent: 'root', + k: 1.5, + isRoot: false, + locked: true, + detached: true, + coordinates: { x: 100, y: 200 }, + colors: { name: '#ff0000', background: '#00ff00', branch: '#0000ff' }, + font: { size: 16, style: 'italic', weight: 'bold' }, + image: { src: 'http://img.png', size: 50 }, + link: { href: 'http://example.com' }, + }); + + const yNode = new Y.Map(); + populateYMapFromNodeProps(yNode, input); + nodesMap.set('n1', yNode); + + const result = yMapToNodeProps(nodesMap.get('n1')!); + + expect(result).toEqual( + expect.objectContaining({ + id: 'n1', + name: 'Hello', + parent: 'root', + k: 1.5, + isRoot: false, + locked: true, + detached: true, + coordinates: { x: 100, y: 200 }, + colors: { + name: '#ff0000', + background: '#00ff00', + branch: '#0000ff', + }, + font: { size: 16, style: 'italic', weight: 'bold' }, + image: { src: 'http://img.png', size: 50 }, + link: { href: 'http://example.com' }, + }) + ); + }); + + it('applies defaults for missing optional properties', () => { + const input: ExportNodeProperties = { + id: 'n2', + parent: undefined, + k: undefined, + name: undefined, + isRoot: undefined, + locked: undefined, + detached: undefined, + coordinates: undefined, + colors: undefined, + font: undefined, + image: undefined, + link: undefined, + } as unknown as ExportNodeProperties; + + const yNode = new Y.Map(); + populateYMapFromNodeProps(yNode, input); + nodesMap.set('n2', yNode); + + const result = yMapToNodeProps(nodesMap.get('n2')!); + + expect(result).toEqual( + expect.objectContaining({ + parent: null, + k: 1, + name: '', + isRoot: false, + locked: false, + detached: false, + coordinates: { x: 0, y: 0 }, + }) + ); + }); +}); + +// ─── Yjs URL building ──────────────────────────────────────── + +describe('Yjs URL building', () => { + let querySelectorSpy: jest.SpyInstance; + + beforeEach(() => { + querySelectorSpy = jest.spyOn(document, 'querySelector'); + }); + + afterEach(() => { + querySelectorSpy.mockRestore(); + }); + + // jsdom default location is http://localhost, so tests use that baseline + it('builds ws URL and uses document base href', () => { + querySelectorSpy.mockReturnValue({ + getAttribute: () => '/', + }); + + const url = buildYjsWsUrl(); + + // jsdom runs on http://localhost -> ws: + expect(url).toBe('ws://localhost/yjs'); + }); + + it('incorporates base href into path', () => { + querySelectorSpy.mockReturnValue({ + getAttribute: () => '/app/', + }); + + const url = buildYjsWsUrl(); + + expect(url).toBe('ws://localhost/app/yjs'); + }); + + it('appends trailing slash to base href if missing', () => { + querySelectorSpy.mockReturnValue({ + getAttribute: () => '/app', + }); + + const url = buildYjsWsUrl(); + + expect(url).toBe('ws://localhost/app/yjs'); + }); + + it('defaults base href to / when no base element', () => { + querySelectorSpy.mockReturnValue(null); + + const url = buildYjsWsUrl(); + + expect(url).toBe('ws://localhost/yjs'); + }); + + it('selects protocol based on page protocol', () => { + // Verify the protocol-selection logic via the method output + // jsdom defaults to http: -> ws:, confirming the mapping works + querySelectorSpy.mockReturnValue(null); + const url = buildYjsWsUrl(); + expect(url).toMatch(/^ws:\/\//); + // The https: -> wss: path uses the same ternary expression + }); +}); + +// ─── Y.Doc property application to MMP ─────────────────────── + +describe('Y.Doc property application to MMP', () => { + it('resolves simple property (name) via reverse mapping', () => { + const updates = resolveMmpPropertyUpdate('name', 'New Name'); + expect(updates).toEqual([{ prop: 'name', val: 'New Name' }]); + }); + + it('resolves simple property (locked) via reverse mapping', () => { + const updates = resolveMmpPropertyUpdate('locked', true); + expect(updates).toEqual([{ prop: 'locked', val: true }]); + }); + + it('resolves compound property (colors) via reverse mapping', () => { + const updates = resolveMmpPropertyUpdate('colors', { + background: '#ff0000', + branch: '#00ff00', + name: '#0000ff', + }); + + expect(updates).toEqual([ + { prop: 'backgroundColor', val: '#ff0000' }, + { prop: 'branchColor', val: '#00ff00' }, + { prop: 'nameColor', val: '#0000ff' }, + ]); + }); + + it('resolves compound property (font) via reverse mapping', () => { + const updates = resolveMmpPropertyUpdate('font', { + size: 20, + weight: 'bold', + style: 'italic', + }); + + expect(updates).toEqual( + expect.arrayContaining([ + { prop: 'fontSize', val: 20 }, + { prop: 'fontWeight', val: 'bold' }, + { prop: 'fontStyle', val: 'italic' }, + ]) + ); + }); + + it('returns empty array for unknown property keys', () => { + const updates = resolveMmpPropertyUpdate('unknown_key', 'value'); + expect(updates).toEqual([]); + }); + + it('handles null compound value gracefully', () => { + const updates = resolveCompoundMmpUpdates( + { background: 'backgroundColor' }, + null as unknown as Record + ); + expect(updates).toEqual([]); + }); +}); + +// ─── Client color resolution ───────────────────────────────── + +describe('client color resolution', () => { + it('returns existing color when no collision', () => { + const result = resolveClientColor( + '#ff0000', + new Set(['#00ff00', '#0000ff']) + ); + expect(result).toBe('#ff0000'); + }); + + it('generates a different valid hex color on collision', () => { + const result = resolveClientColor('#00ff00', new Set(['#00ff00'])); + expect(result).toMatch(/^#(?!00ff00)[0-9a-f]{6}$/); + }); + + it('handles empty used colors set', () => { + const result = resolveClientColor('#ff0000', new Set()); + expect(result).toBe('#ff0000'); + }); +}); + +// ─── findAffectedNodes ─────────────────────────────────────── + +describe('findAffectedNodes', () => { + it('collects node IDs from both old and new mappings', () => { + const oldMapping = { + c1: { nodeId: 'node-a', color: '#ff0000' }, + c2: { nodeId: 'node-b', color: '#00ff00' }, + }; + + const newMapping = { + c1: { nodeId: 'node-b', color: '#ff0000' }, + c3: { nodeId: 'node-c', color: '#0000ff' }, + }; + + const result = findAffectedNodes(oldMapping, newMapping); + + expect(result).toEqual(new Set(['node-a', 'node-b', 'node-c'])); + }); + + it('excludes empty nodeId strings', () => { + const oldMapping = { + c1: { nodeId: '', color: '#ff0000' }, + }; + + const newMapping = { + c1: { nodeId: 'node-a', color: '#ff0000' }, + }; + + const result = findAffectedNodes(oldMapping, newMapping); + + expect(result).toEqual(new Set(['node-a'])); + }); + + it('returns empty set when no nodes selected', () => { + const oldMapping = { + c1: { nodeId: '', color: '#ff0000' }, + }; + + const newMapping = { + c1: { nodeId: '', color: '#ff0000' }, + }; + + const result = findAffectedNodes(oldMapping, newMapping); + + expect(result.size).toBe(0); + }); +}); + +// ─── sortParentFirst ───────────────────────────────────────── + +describe('sortParentFirst', () => { + it('places root node first when children appear before parent', () => { + const child = createMockNode({ + id: 'child-1', + parent: 'root-1', + isRoot: false, + }); + const root = createMockNode({ id: 'root-1', parent: '', isRoot: true }); + + const result = sortParentFirst([child, root]); + + expect(result.map(n => n.id)).toEqual(['root-1', 'child-1']); + }); + + it('ensures grandchild nodes come after their parent', () => { + const grandchild = createMockNode({ + id: 'gc-1', + parent: 'child-1', + isRoot: false, + }); + const child = createMockNode({ + id: 'child-1', + parent: 'root-1', + isRoot: false, + }); + const root = createMockNode({ id: 'root-1', parent: '', isRoot: true }); + + const result = sortParentFirst([grandchild, child, root]); + + expect(result.map(n => n.id)).toEqual(['root-1', 'child-1', 'gc-1']); + }); + + it('handles already-sorted input without changing order', () => { + const root = createMockNode({ id: 'root-1', parent: '', isRoot: true }); + const child1 = createMockNode({ + id: 'c1', + parent: 'root-1', + isRoot: false, + }); + const child2 = createMockNode({ + id: 'c2', + parent: 'root-1', + isRoot: false, + }); + + const result = sortParentFirst([root, child1, child2]); + + expect(result.map(n => n.id)).toEqual(['root-1', 'c1', 'c2']); + }); + + it('returns original array when no root is found', () => { + const node1 = createMockNode({ id: 'n1', parent: 'n2', isRoot: false }); + const node2 = createMockNode({ id: 'n2', parent: 'n1', isRoot: false }); + + const result = sortParentFirst([node1, node2]); + + expect(result.map(n => n.id)).toEqual(['n1', 'n2']); + }); + + it('groups sibling nodes under their shared parent', () => { + const root = createMockNode({ id: 'root', parent: '', isRoot: true }); + const b = createMockNode({ id: 'b', parent: 'root', isRoot: false }); + const a = createMockNode({ id: 'a', parent: 'root', isRoot: false }); + const bChild = createMockNode({ + id: 'b-child', + parent: 'b', + isRoot: false, + }); + + const result = sortParentFirst([bChild, a, b, root]); + + expect(result[0].id).toBe('root'); + expect(result.indexOf(b)).toBeLessThan(result.indexOf(bChild)); + }); + + it('returns empty array for empty input', () => { + const result = sortParentFirst([]); + + expect(result).toEqual([]); + }); + + it('appends orphaned nodes not reachable from root', () => { + const root = createMockNode({ id: 'root', parent: '', isRoot: true }); + const child = createMockNode({ + id: 'child', + parent: 'root', + isRoot: false, + }); + const orphan = createMockNode({ + id: 'orphan', + parent: 'deleted-parent', + isRoot: false, + }); + + const result = sortParentFirst([orphan, child, root]); + + expect(result.map(n => n.id)).toEqual(['root', 'child', 'orphan']); + }); +}); + +// ─── collectDescendantIds ──────────────────────────────────── + +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/yjs-utils.ts b/teammapper-frontend/src/app/core/services/map-sync/yjs-utils.ts index ac1dca2b7..831e0e0b4 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 @@ -2,8 +2,6 @@ import * as Y from 'yjs'; import { ExportNodeProperties } from '@mmp/map/types'; import { ReversePropertyMapping } from './server-types'; -const MESSAGE_WRITE_ACCESS = 4; - export type ClientColorMapping = Record; export interface ClientColorMappingValue { @@ -73,14 +71,6 @@ export function buildYjsWsUrl(): string { return `${protocol}//${host}${path}yjs`; } -// Returns true for writable, false for read-only, null if not a write-access message -export function parseWriteAccessBytes(data: Uint8Array): boolean | null { - if (data.length >= 2 && data[0] === MESSAGE_WRITE_ACCESS) { - return data[1] === 1; - } - return null; -} - export function resolveClientColor( currentColor: string, usedColors: Set @@ -181,6 +171,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 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 aa152f26a..ecc1419b6 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 758853e96..05f5ce058 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 @@