diff --git a/demo/smoke_device_position_history.mjs b/demo/smoke_device_position_history.mjs index 4edf802e..4d939e63 100644 --- a/demo/smoke_device_position_history.mjs +++ b/demo/smoke_device_position_history.mjs @@ -97,12 +97,24 @@ const res = await page.evaluate(async () => { out.futureFieldsSurviveDrag = c._layout[deviceId].k === 0 && c._layout[deviceId].future === 'kept'; out.undoEnabled = !sr().querySelector('[data-device-position-history="undo"]').disabled; + // #397 AC1: after a write the local copy IS what went over the wire. + out.localCopyEqualsTheWire = writes.length > 0 + && JSON.stringify(c._layout[deviceId]) === JSON.stringify({ + ...c._layout[deviceId], ...writes[writes.length - 1].pos, + }) + && JSON.stringify(serverLayout[deviceId]) === JSON.stringify(writes[writes.length - 1].pos); const afterDrag = structuredClone(c._layout[deviceId]); sr().querySelector('[data-device-position-history="undo"]').click(); await idle(); - out.undoRestoresExactStart = c._layout[deviceId].x === explicit.x - && c._layout[deviceId].y === explicit.y + // #397: the card now keeps what it sent — the canonical position — so a + // restored placement may differ from the raw `explicit` by the lattice snap + // (< 1e-9 of the plan, invisible). The equality is therefore stated to that + // precision, and the snap itself is pinned separately below: a real logical + // drift would exceed it by orders of magnitude. + const near = (a, b) => Math.abs(a - b) < 1e-9; + out.undoRestoresExactStart = near(c._layout[deviceId].x, explicit.x) + && near(c._layout[deviceId].y, explicit.y) && c._layout[deviceId].k === 0 && c._devicePositionHistory.canRedo; sr().querySelector('[data-device-position-history="redo"]').click(); @@ -113,7 +125,7 @@ const res = await page.evaluate(async () => { })); await idle(); out.keyboardUndoWorks = c._devicePositionHistory.canRedo - && c._layout[deviceId].x === explicit.x && c._layout[deviceId].y === explicit.y; + && near(c._layout[deviceId].x, explicit.x) && near(c._layout[deviceId].y, explicit.y); window.dispatchEvent(new KeyboardEvent('keydown', { key: 'z', code: 'KeyZ', ctrlKey: true, shiftKey: true, bubbles: true, composed: true, cancelable: true, @@ -231,10 +243,22 @@ const res = await page.evaluate(async () => { out.nativeInputHistoryNotIntercepted = c._devicePositionHistory.size === sizeBeforeInput; input.remove(); - // Same-content reload is an own/reconnect echo; different content is remote authority. - serverLayout = structuredClone(c._layout); + // Same-content reload is an own/reconnect echo; different content is remote + // authority. #397: the server snapshot is NOT copied from the card here — it + // already holds what actually went over the wire (the fake WS above stores + // `message.pos`). The former `serverLayout = structuredClone(c._layout)` + // erased by hand the very divergence this check exists to catch: the wire + // carries the canonical position while `_layout` kept the raw one, so the + // card mistook its own echo for a remote edit and cleared the stack. With + // the assignment gone the check reddens on the unfixed code. + const ownEchoServerPos = structuredClone(serverLayout[deviceId]); + const ownEchoLocalPos = structuredClone(c._layout[deviceId]); await c._reloadLayoutOnly(); out.sameContentReloadKeepsHistory = c._devicePositionHistory.canUndo; + // The point of the check is only meaningful if the two sides were equal to + // begin with: pin that explicitly instead of assuming it. + out.ownEchoMatchesWhatWentOverTheWire = + JSON.stringify(ownEchoServerPos) === JSON.stringify(ownEchoLocalPos); serverLayout = { ...serverLayout, [deviceId]: { ...serverLayout[deviceId], x: serverLayout[deviceId].x + 0.01 }, @@ -243,6 +267,40 @@ const res = await page.evaluate(async () => { out.remoteContentClearsHistory = !c._devicePositionHistory.canUndo && !c._devicePositionHistory.canRedo; + // #397 AC5b: the echo of a DELETE must not clear the stack either — the + // delete branch removes a key instead of replacing a value, so proving the + // update branch says nothing about it. + const echoProbe = c._devices.find((candidate) => candidate.id !== deviceId + && candidate.bindingStatus?.kind !== 'ha_disabled'); + if (echoProbe) { + c._layout = { ...c._layout, [echoProbe.id]: { s: echoProbe.space, x: 0.42, y: 0.42 } }; + await c._persistDevicePlacement(echoProbe.id, { s: echoProbe.space, x: 0.42, y: 0.42 }); + c._devicePositionHistory.push({ + name: 'echo probe move', + before: { deviceId: echoProbe.id, spaceId: echoProbe.space, placement: null }, + after: { deviceId: echoProbe.id, spaceId: echoProbe.space, + placement: { s: echoProbe.space, x: 0.42, y: 0.42 } }, + }); + await c._persistDevicePlacement(echoProbe.id, null); + await c._reloadLayoutOnly(); + out.deleteEchoKeepsHistory = c._devicePositionHistory.canUndo + && c._layout[echoProbe.id] === undefined; + + // #397 AC7: while a write is in flight the card is the authority — a + // server answer holding the OLD position must not win the merge. + const inFlightPos = { s: echoProbe.space, x: 0.63, y: 0.21 }; + serverLayout = { ...serverLayout, [echoProbe.id]: { s: echoProbe.space, x: 0.1, y: 0.1 } }; + c._sentPos.set(echoProbe.id, structuredClone(inFlightPos)); + c._layout = { ...c._layout, [echoProbe.id]: structuredClone(inFlightPos) }; + await c._reloadLayoutOnly(); + out.inFlightPositionWinsTheMerge = + JSON.stringify(c._layout[echoProbe.id]) === JSON.stringify(inFlightPos); + c._sentPos.delete(echoProbe.id); + } else { + out.deleteEchoKeepsHistory = null; + out.inFlightPositionWinsTheMerge = null; + } + // A valid command owns its original space and makes the result visible there. const otherDevice = c._devices.find((candidate) => candidate.space !== device.space && candidate.bindingStatus?.kind !== 'ha_disabled'); diff --git a/docs/CHANGELOG.md b/docs/CHANGELOG.md index aaf5d06f..ba789f0b 100644 --- a/docs/CHANGELOG.md +++ b/docs/CHANGELOG.md @@ -2,6 +2,11 @@ ## Unreleased +- Undo/Redo of marker positions no longer goes dark on its own: the card now + keeps exactly what it sent to the server, so a reconnect or a second tab + stops looking like someone else's edit + ([#397](https://github.com/Matysh/houseplan-card/issues/397)). + - Dangerous actions now use one accessible House Plan confirmation dialog instead of the browser prompt. Device, draft, plan and space deletion plus lock opening share clear consequences, safe Cancel/X/Escape behaviour and diff --git a/docs/CHANGELOG.ru.md b/docs/CHANGELOG.ru.md index 59485b0f..93eb5347 100755 --- a/docs/CHANGELOG.ru.md +++ b/docs/CHANGELOG.ru.md @@ -8,6 +8,11 @@ ## Не выпущено +- История Undo/Redo позиций маркеров больше не гаснет сама: карточка хранит + ровно то, что отправила серверу, поэтому переподключение или вторая вкладка + перестали выглядеть чужой правкой + ([#397](https://github.com/Matysh/houseplan-card/issues/397)). + - Опасные действия теперь используют единый доступный диалог подтверждения House Plan вместо системного окна браузера. Удаление устройства, черновика, файла плана и пространства, а также открытие замка получили понятные diff --git a/scripts/mutation-gate.mjs b/scripts/mutation-gate.mjs index ade69499..a9629ef9 100644 --- a/scripts/mutation-gate.mjs +++ b/scripts/mutation-gate.mjs @@ -746,6 +746,18 @@ const MUTANT_DEFINITIONS = [ replace: " this._persistDecorStyle();\n }, 0);", }], }, + { + id: 'device-echo-keeps-local-noncanonical', + guard: 'node demo/smoke_device_position_history.mjs', + because: 'a card that keeps the raw position while sending the canonical ' + + 'one mistakes its own echo for a remote edit and wipes the undo stack ' + + '(#397 B3)', + patches: [{ + file: 'src/houseplan-card.ts', + find: ' this._layout = { ...this._layout, [deviceId]: pos };', + replace: ' void pos;', + }], + }, { id: 'camera-cancel-loses-zoom', guard: 'node demo/smoke_smooth_zoom.mjs', diff --git a/src/houseplan-card.ts b/src/houseplan-card.ts index 3ca1dae1..1f7a52a2 100755 --- a/src/houseplan-card.ts +++ b/src/houseplan-card.ts @@ -5241,7 +5241,12 @@ export class HouseplanCard extends LitElement { type: 'houseplan/layout/delete', device_id: deviceId, }); } else { + // #397: keep locally exactly what goes on the wire, or the card's + // own echo reads as a remote edit and wipes the undo stack. const pos = canonicalizePosition(this._layout[deviceId]); + if (contentFingerprint(pos) !== contentFingerprint(this._layout[deviceId])) { + this._layout = { ...this._layout, [deviceId]: pos }; + } pending = pos; this._sentPos.set(deviceId, pending); registered = true; diff --git a/test/device-position-echo.test.mjs b/test/device-position-echo.test.mjs new file mode 100644 index 00000000..a39cacac --- /dev/null +++ b/test/device-position-echo.test.mjs @@ -0,0 +1,48 @@ +import test from 'node:test'; +import assert from 'node:assert/strict'; +import { readFileSync } from 'node:fs'; +import { canonicalizePosition } from '../test-build/coordinate-canonicalization.js'; +import { contentFingerprint } from '../test-build/visual-continuity.js'; + +// #397: the card sends the canonical position to the server and must keep the +// same value locally. While it kept the raw one, its own echo came back +// looking foreign — the fingerprints differed — and the next reload wiped the +// undo history the user had just filled (AC10 of #74). + +const CARD = readFileSync(new URL('../src/houseplan-card.ts', import.meta.url), 'utf8'); + +test('#397: the premise holds — canonicalization is not identity', () => { + // If it were, the whole issue would be moot and the guards below decorative. + const raw = { s: 'ground', x: 0.024999999999999942, y: 0.63 / 3 }; + const canonical = canonicalizePosition(raw); + assert.notEqual(contentFingerprint(canonical), contentFingerprint(raw), + 'the lattice snap must actually change the value'); + assert.ok(Math.abs(canonical.x - raw.x) < 1e-9, + 'and it must be a snap, not a move — the difference is invisible on screen'); +}); + +test('#397 AC1: the update branch stores what it sends, before sending it', () => { + const branch = CARD.slice( + CARD.indexOf('private async _persistDevicePlacement'), + CARD.indexOf('this._persistLocalLayout();', CARD.indexOf('private async _persistDevicePlacement')), + ); + assert.ok(branch, 'the persist method must be found'); + const write = branch.indexOf('this._layout = { ...this._layout, [deviceId]: pos }'); + const send = branch.indexOf("type: 'houseplan/layout/update'"); + const fingerprint = branch.indexOf('this._layoutContentFingerprint = contentFingerprint(this._layout)'); + assert.ok(write > 0, 'the canonical position must be written back into _layout'); + assert.ok(write < send, + 'the local copy is updated BEFORE the wire, so a reload racing the answer ' + + 'sees the value that was sent'); + assert.ok(send < fingerprint, + 'the fingerprint is taken after the write, over the canonical layout'); +}); + +test('#397 AC5a: the delete branch removes the key before the fingerprint', () => { + const method = CARD.slice(CARD.indexOf('private async _persistDevicePlacement')); + const apply = method.indexOf('applyDevicePlacement(this._layout, deviceId, placement)'); + const fingerprint = method.indexOf('this._layoutContentFingerprint = contentFingerprint(this._layout)'); + assert.ok(apply > 0 && apply < fingerprint, + 'both branches mutate _layout through applyDevicePlacement before the ' + + 'fingerprint is recorded — deletion removes the key, not replaces a value'); +});