The inline editor commits by record identity, not viewport position (improvise-2wu3)
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VdfEGXA7aJ6ftMsCc9PJze
This commit is contained in:
+27
-3
@@ -909,7 +909,8 @@ gotcha 47 asks for — closing e24v has to come to that line and delete it.
|
||||
aggregate, which is true in both front ends because the browser's refusal
|
||||
names drill. Delivering drill is `improvise-obtu`'s, not this bullet's.
|
||||
- **The commit's write is not spelled again here.** It is
|
||||
`Client::typed_cell`'s, the same one click-to-edit uses, so a browser keyboard
|
||||
`typed_cell_command`'s, the same one click-to-edit reaches through
|
||||
`typed_record_frame`, so a browser keyboard
|
||||
commit and a browser mouse commit cannot disagree about what `""` or `"42"`
|
||||
means.
|
||||
- **`cell_at_cursor` returns `None` when the selection is scrolled out of the
|
||||
@@ -1374,8 +1375,30 @@ withholds rather than invents).
|
||||
from the host, which is the side that formats it; guessing it locally is how the
|
||||
two ends drift apart.
|
||||
|
||||
**The page commits by identity, never by position (improvise-2wu3).** While an
|
||||
inline editor is open, `repaint` returns early, so the screen is frozen. But
|
||||
`on_frame` goes on folding, so the cache moves while the screen does not. The
|
||||
page used to keep the editor's viewport `(row, col)` and resolve it against the
|
||||
**current** cache at commit. If another client had hidden a row above, that
|
||||
position now held a different record. `basis.claim` was satisfied too, because
|
||||
the fresh `Projection::Basis` had been folded along with the rows. So the write
|
||||
landed on a row the user never looked at, and it was reported accepted. The
|
||||
basis was current *for the client*; the stale thing was the screen, and
|
||||
nothing measures that.
|
||||
|
||||
So the JS surface is now two calls. `edit_target(row, col)` returns the
|
||||
`RecordCell` as an opaque string, and the page takes it when the editor opens.
|
||||
`typed_cell(target, text)` composes the write against that surrogate
|
||||
(`typed_record_frame`). The position-taking composer is gone from the page's
|
||||
surface, so the confusion cannot be written in the page. `typed_cell_frame(row,
|
||||
col, …)` survives as Rust-only sugar over `record_cell_at` and
|
||||
`typed_record_frame`, for tests that compose at the moment they read. The
|
||||
surrogate cannot go stale in the way a position can: if the record is written
|
||||
by someone else meanwhile, the basis tier refuses it (`RecordMoved`); if it is
|
||||
gone, `RecordGone`. Either way the user is told.
|
||||
|
||||
`typed_cell_command` is the one composition shared by the mouse path
|
||||
(`typed_cell_frame`) and the keyboard path (`keys::commit`). Writing it twice is
|
||||
(`typed_record_frame`) and the keyboard path (`keys::commit`). Writing it twice is
|
||||
how a browser commit by Enter and a browser commit by click would come to
|
||||
disagree about what an empty string means.
|
||||
|
||||
@@ -1524,7 +1547,8 @@ whether a typed string is a number. Five calls are the whole harness:
|
||||
| `client.set_viewport(rows, cols)` | the handshake frame, and every resize; also records the window so keys resolve here |
|
||||
| `client.on_frame(json)` | fold one frame, in arrival order |
|
||||
| `client.render_model()` | everything to paint, already decided |
|
||||
| `client.typed_cell(r, c, s)` | the frame for "the user typed s here" |
|
||||
| `client.edit_target(r, c)` | which stored cell an editor opened here edits: an opaque string the page holds, or `null` |
|
||||
| `client.typed_cell(target, s)` | the frame for "the user typed s into that cell", by identity (improvise-2wu3) |
|
||||
| `client.on_key_event(k,c,a,s,m)` | the frame for a keypress, folded locally first where this client may answer it |
|
||||
|
||||
**The two rules this file must not break are ordering and framing**: frames are
|
||||
|
||||
@@ -4353,3 +4353,39 @@ Records mode is deliberately not covered. There the TUI yanks `None` and its
|
||||
`p` writes a junk `_Dim`/`_Index` cell, because it resolves the synthetic key
|
||||
(improvise-6p8q). The browser is right and the TUI is wrong, so a lockstep
|
||||
case there would assert the bug.
|
||||
|
||||
### an_edit_writes_the_cell_the_user_clicked_after_the_rows_renumber
|
||||
|
||||
improvise-2wu3. `web/app.js` freezes the screen while an inline editor is open
|
||||
(`repaint` returns early), but `on_frame` goes on folding. The editor kept its
|
||||
viewport `(row, col)`, and `typed_cell` resolved it against the *current*
|
||||
cache at commit.
|
||||
|
||||
The test follows the ticket's acceptance criteria against a real `Host`:
|
||||
1. Client A reads the North/Revenue record at `(0, 0)`.
|
||||
2. Client B's `HideItem("North")` is applied.
|
||||
3. Only the frames addressed to A are folded into A, with no repaint, just as
|
||||
the page does it.
|
||||
4. A precondition asserts the rows really renumbered.
|
||||
5. The write A composes must name the record A clicked.
|
||||
|
||||
**Red first**, at HEAD after improvise-ktlk (PR #169), which moved the editor's
|
||||
seed but not its commit. Written against the position API as it stood
|
||||
(`typed_cell(0, 0, "42")`), it failed. The write named `RecordHash(161215…)`,
|
||||
which is South's record now sitting at row 0, where North's `RecordHash(246296…)`
|
||||
was expected. So the ticket still reproduced. The frame would have passed the
|
||||
basis check, because A had folded the fresh `Projection::Basis` along with the
|
||||
rows.
|
||||
|
||||
**The fix makes the fault unwritable in the page.** The page's surface no
|
||||
longer takes a position at commit. `edit_target(row, col)` is read when the
|
||||
editor opens and returns the `RecordCell` as an opaque string, and
|
||||
`typed_cell(target, text)` composes against it. The same test, rewritten for
|
||||
that surface, passes. There is no position left at commit for a fault to
|
||||
re-introduce. Rust-side `typed_cell_frame(row, col, …)` remains for tests
|
||||
that compose at read time.
|
||||
|
||||
The page path is exercised end to end by `harness/tests/shell.spec.js`'s
|
||||
"committing an edit sends one frame and shows the host's value", which goes
|
||||
through `edit_target` and the new `typed_cell`. The whole harness passes
|
||||
locally (69 passed, 4 skipped).
|
||||
|
||||
@@ -135,8 +135,12 @@ fn a_browser_client_seeds_types_a_number_and_sees_it_come_back() {
|
||||
this fixture has no group headers, so the two row coordinates \
|
||||
coincide here — see a_grouped_row_axis_does_not_shift_the_write"
|
||||
);
|
||||
let target = client
|
||||
.edit_target(at.data_row, at.col)
|
||||
.expect("serialization must not fail")
|
||||
.expect("an editable cell has an edit target");
|
||||
let frame = client
|
||||
.typed_cell(at.data_row, at.col, "7500")
|
||||
.typed_cell(&target, "7500")
|
||||
.expect("serialization must not fail")
|
||||
.expect("an editable cell must compose a write");
|
||||
let frames = send(&mut host, id, &frame);
|
||||
@@ -218,8 +222,12 @@ fn a_grouped_row_axis_does_not_shift_the_write() {
|
||||
coordinates coincide and nothing below is being tested"
|
||||
);
|
||||
|
||||
let target = client
|
||||
.edit_target(at.data_row, at.col)
|
||||
.expect("serialization must not fail")
|
||||
.expect("an editable cell has an edit target");
|
||||
let frame = client
|
||||
.typed_cell(at.data_row, at.col, "4200")
|
||||
.typed_cell(&target, "4200")
|
||||
.expect("serialization must not fail")
|
||||
.expect("an editable cell must compose a write");
|
||||
send(&mut host, id, &frame);
|
||||
@@ -287,9 +295,9 @@ fn an_aggregated_cell_reaches_the_page_as_not_editable() {
|
||||
"every cell here aggregates over Product, so none may be written"
|
||||
);
|
||||
assert_eq!(
|
||||
client.typed_cell(0, 0, "42").unwrap(),
|
||||
client.edit_target(0, 0).unwrap(),
|
||||
None,
|
||||
"and typing into one composes nothing, because the id is of the wrong kind"
|
||||
"and there is no cell to type into, because the id is of the wrong kind"
|
||||
);
|
||||
}
|
||||
|
||||
@@ -649,7 +657,13 @@ fn an_edit_writes_the_cell_the_user_clicked_after_the_rows_renumber() {
|
||||
let (b, _) = host.connect(10, 10);
|
||||
|
||||
// A opens an editor on the North/Revenue cell it is looking at.
|
||||
let clicked = editor.record_cell_at(0, 0).expect("North/Revenue is stored");
|
||||
let clicked = editor
|
||||
.record_cell_at(0, 0)
|
||||
.expect("North/Revenue is stored");
|
||||
let target = editor
|
||||
.edit_target(0, 0)
|
||||
.expect("serialization must not fail")
|
||||
.expect("a stored cell has an edit target");
|
||||
|
||||
// B hides North; A folds the result without repainting, as the page does.
|
||||
let view = host.app().view_name().to_string();
|
||||
@@ -671,15 +685,23 @@ fn an_edit_writes_the_cell_the_user_clicked_after_the_rows_renumber() {
|
||||
);
|
||||
|
||||
let json = editor
|
||||
.typed_cell(0, 0, "42")
|
||||
.typed_cell(&target, "42")
|
||||
.expect("serialization must not fail")
|
||||
.expect("a stored cell composes a write");
|
||||
let ClientFrame::Commands(cmds) = serde_json::from_str(&json).expect("a client frame") else {
|
||||
panic!("a write is a Commands frame");
|
||||
};
|
||||
let written = match cmds.as_slice() {
|
||||
[Command::Model { cmd: ModelCommand::SetCell { cell, .. }, .. }] => cell.clone(),
|
||||
[
|
||||
Command::Model {
|
||||
cmd: ModelCommand::SetCell { cell, .. },
|
||||
..
|
||||
},
|
||||
] => cell.clone(),
|
||||
other => panic!("expected one SetCell, got {other:?}"),
|
||||
};
|
||||
assert_eq!(written, clicked, "the write landed on a row the user never looked at");
|
||||
assert_eq!(
|
||||
written, clicked,
|
||||
"the write landed on a row the user never looked at"
|
||||
);
|
||||
}
|
||||
|
||||
+7
-3
@@ -541,6 +541,10 @@ function gridTable(model) {
|
||||
* back is the server's, never the string typed here. */
|
||||
function edit(td, row, col, seed) {
|
||||
if (editing) return;
|
||||
// WHICH cell, captured now: rows renumber while the editor is open, so the
|
||||
// position is not an address by commit time (improvise-2wu3).
|
||||
const target = client.edit_target(row, col);
|
||||
if (!target) return;
|
||||
// `seed` is `RenderCell::edit_text` -- the STORED value, not the drawn one:
|
||||
// `td.textContent` has been through the number format.
|
||||
const original = seed;
|
||||
@@ -563,9 +567,9 @@ function edit(td, row, col, seed) {
|
||||
closed = true;
|
||||
editing = null;
|
||||
if (commit) {
|
||||
const frame = client.typed_cell(row, col, input.value);
|
||||
// null only if the grid moved under the editor; the repaint that
|
||||
// follows restores whatever the server now says is there.
|
||||
const frame = client.typed_cell(target, input.value);
|
||||
// null only before any basis arrived; the repaint that follows
|
||||
// restores whatever the server now says is there.
|
||||
if (frame) transport.send(frame);
|
||||
}
|
||||
td.replaceChildren(drawn);
|
||||
|
||||
Reference in New Issue
Block a user