mirror of
https://github.com/Matysh/houseplan-card
synced 2026-10-02 12:49:56 +00:00
fix: the card keeps the position it sent, so its echo is not foreign (#397)
B3: _persistDevicePlacement sent canonicalizePosition(...) to the server and left the raw value in _layout, then recorded the fingerprint over that raw snapshot. Canonicalization is not identity — it snaps to the lattice — so 39 of 115 pixel-derived coordinates differ, and the next _reloadLayoutOnly or _adoptStructuralResponses saw its own write as a remote edit: history cleared, _layout replaced. The old _persistLayout wrote the canonical value back; the per-device path introduced by #74 lost that line. M1: the smoke that was supposed to prove AC10 assigned serverLayout = structuredClone(c._layout) right before the reload — erasing by hand the very divergence it existed to catch, so it could not fail. The fake WS already stores what went over the wire; the assignment is gone and the check now reddens on the unfixed code (verified: three checks red without the fix, including this one). Also proven, because the fix touches their neighbourhood: the echo of a DELETE keeps the history (the branch removes a key rather than replacing a value), and an in-flight write still wins the merge against a server answer holding the old position. One existing assertion was loosened deliberately: undo now restores a position that may differ from the raw one by the lattice snap (<1e-9 of the plan). That is the point of the fix — local and server agree — so the equality is stated to that precision, with the snap size pinned separately so a real drift would still fail. User-Visible: yes Issue: #397
This commit is contained in:
@@ -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');
|
||||
|
||||
@@ -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
|
||||
|
||||
@@ -8,6 +8,11 @@
|
||||
|
||||
## Не выпущено
|
||||
|
||||
- История Undo/Redo позиций маркеров больше не гаснет сама: карточка хранит
|
||||
ровно то, что отправила серверу, поэтому переподключение или вторая вкладка
|
||||
перестали выглядеть чужой правкой
|
||||
([#397](https://github.com/Matysh/houseplan-card/issues/397)).
|
||||
|
||||
- Опасные действия теперь используют единый доступный диалог подтверждения
|
||||
House Plan вместо системного окна браузера. Удаление устройства, черновика,
|
||||
файла плана и пространства, а также открытие замка получили понятные
|
||||
|
||||
@@ -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',
|
||||
|
||||
@@ -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;
|
||||
|
||||
@@ -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');
|
||||
});
|
||||
Reference in New Issue
Block a user