From b872923331e3ea0bcafbe94a26a7eb01ee29a9d7 Mon Sep 17 00:00:00 2001 From: Cursor Agent Date: Sun, 27 Sep 2026 16:51:50 +0000 Subject: [PATCH] fix(improvise-2mqz): export the requesting session view Co-authored-by: Ed Langley --- .../src/command/cmd/commit.rs | 12 +-- crates/improvise-command/src/ui/host.rs | 81 ++++++++++++++++-- .../src/ui/reduce/vocabulary_is_complete.rs | 14 +--- crates/improvise-io/src/persistence/export.rs | 5 +- crates/improvise-protocol/src/frame.rs | 28 +++---- crates/improvise-protocol/src/keymap.rs | 6 +- crates/improvise-protocol/src/resolve.rs | 84 +++++++++++-------- crates/improvise-wasm-client/src/socket.rs | 10 +-- .../tests/post_message.rs | 12 ++- web/tests/bundle/wasm_surface.test.js | 3 + 10 files changed, 162 insertions(+), 93 deletions(-) diff --git a/crates/improvise-command/src/command/cmd/commit.rs b/crates/improvise-command/src/command/cmd/commit.rs index 59f88a70..7ed8d269 100644 --- a/crates/improvise-command/src/command/cmd/commit.rs +++ b/crates/improvise-command/src/command/cmd/commit.rs @@ -1275,10 +1275,8 @@ impl Cmd for CommitSave { effect::change_mode(AppMode::Normal), ]; } - // The mode change and buffer clear come **first**, or a failed `SaveAs` - // aborts with the minibuffer still drawn over its error (Copilot, PR #27). - // Buffer clear added for improvise-2mqz: the keymap uses rebind instead of - // commit (because LocalThenRequest can't be in a Binding::Sequence). + // Clear before `SaveAs`, whose failure aborts; this command owns the clear + // because its LocalThenRequest cannot live in a sequence (improvise-2mqz). vec![ Box::new(effect::SetBuffer { name: "save".into(), @@ -1305,10 +1303,8 @@ impl Cmd for CommitExport { effect::change_mode(AppMode::Normal), ]; } - // The mode change and buffer clear come **first**, or a failed export - // aborts with the minibuffer still drawn over its error (gotcha 21). - // Buffer clear added for improvise-2mqz: the keymap uses rebind instead of - // commit (because LocalThenRequest can't be in a Binding::Sequence). + // Clear before export, whose failure aborts; this command owns the clear + // because its LocalThenRequest cannot live in a sequence (improvise-2mqz). vec![ Box::new(effect::SetBuffer { name: "export".into(), diff --git a/crates/improvise-command/src/ui/host.rs b/crates/improvise-command/src/ui/host.rs index 42ac5193..7d83787e 100644 --- a/crates/improvise-command/src/ui/host.rs +++ b/crates/improvise-command/src/ui/host.rs @@ -118,19 +118,25 @@ impl Host { /// Serialize the document to `.improv` format and return the bytes for download /// (improvise-2mqz). Silent on success, Rejected on failure; no session state moves. fn download_document(&mut self, id: SessionId, filename: String) -> Vec { - let bytes = improvise_io::persistence::format_md(&self.app.model_state.workbook) - .into_bytes(); + let bytes = + improvise_io::persistence::format_md(&self.app.model_state.workbook).into_bytes(); vec![Addressed { to: id, msg: ServerFrame::DocumentBytes { filename, bytes }, }] } - /// Render the active view as CSV and return the bytes for download - /// (improvise-2mqz). Silent on success, Rejected on failure; no session state moves. + /// Render the requesting session's view as downloadable CSV bytes. + /// Silent on success, Rejected on failure (improvise-2mqz). fn download_csv(&mut self, id: SessionId, filename: String) -> Vec { - let view_name = &self.app.model_state.workbook.active_view; - match improvise_io::persistence::render_csv(&self.app.model_state.workbook, view_name) { + let Some(view_name) = self + .sessions + .get(id) + .map(|session| session.view().active_view.clone()) + else { + return Vec::new(); + }; + match improvise_io::persistence::render_csv(&self.app.model_state.workbook, &view_name) { Ok(bytes) => vec![Addressed { to: id, msg: ServerFrame::CsvBytes { filename, bytes }, @@ -464,9 +470,8 @@ mod tests { } } } - // Neither answer carries state: one applied nothing, and what - // the other applied arrived as projections before it. - // DocumentBytes and CsvBytes are for downloads and carry no state. + // These carry no foldable state: downloads are consumed by the + // browser, while other results arrived as projections or did nothing. ServerFrame::Rejected(_) | ServerFrame::Accepted | ServerFrame::DocumentBytes { .. } @@ -2746,6 +2751,64 @@ mod tests { ); } + /// improvise-2mqz: view selection belongs to a session, so an export must + /// not silently use the document's default view or another client's view. + #[test] + fn a_csv_download_exports_the_requesting_sessions_active_view() { + let mut host = host(); + let second = host.app.model_state.workbook.create_view("Second"); + second.set_axis("Region", Axis::Row); + second.set_axis("_Measure", Axis::Column); + second.hide_item("Region", "South"); + let (requester, _) = host.connect(10, 10); + let (bystander, _) = host.connect(10, 10); + + host.handle( + requester, + ClientFrame::command(Command::SwitchView("Second".into())), + ); + assert_eq!( + host.sessions().get(requester).unwrap().view().active_view, + "Second", + "setup: the requester must be on its session-local view" + ); + assert_eq!( + host.sessions().get(bystander).unwrap().view().active_view, + "Default", + "setup: the second session must remain on the default view" + ); + assert_eq!( + host.app().model_state.workbook.active_view, + "Default", + "setup: the document's active view must differ from the requester" + ); + + let answer = host.handle( + requester, + ClientFrame::DownloadCsv { + filename: "second.csv".into(), + }, + ); + let expected = + improvise_io::persistence::render_csv(&host.app().model_state.workbook, "Second") + .unwrap(); + let default = + improvise_io::persistence::render_csv(&host.app().model_state.workbook, "Default") + .unwrap(); + assert_ne!(expected, default, "setup: the two exports must differ"); + assert_eq!( + to(&answer, requester), + vec![ServerFrame::CsvBytes { + filename: "second.csv".into(), + bytes: expected, + }] + ); + assert!( + to(&answer, bystander).is_empty(), + "the download is owed only to its requester: {answer:?}" + ); + } + // ── disconnect ────────────────────────────────────────────────────── /// AC 4: a dropped client stops being projected to, and a late frame diff --git a/crates/improvise-command/src/ui/reduce/vocabulary_is_complete.rs b/crates/improvise-command/src/ui/reduce/vocabulary_is_complete.rs index c08c668d..473825f7 100644 --- a/crates/improvise-command/src/ui/reduce/vocabulary_is_complete.rs +++ b/crates/improvise-command/src/ui/reduce/vocabulary_is_complete.rs @@ -16,8 +16,6 @@ enum Verdict { Frame(&'static str), /// Deliberately off the wire, and the argument. Off(&'static str), - /// Missing, and the ticket that owns closing it. - Gap(&'static str), } /// Checked in all directions: an entry must name a real effect, must not be on @@ -29,14 +27,8 @@ const NOT_ON_THE_WIRE: &[(&str, Verdict)] = &[ // Its text; the refusal flag is one host batch's outcome (context/code/cli.md §10). ("Refuse", Verdict::Command("SetStatus")), ("Save", Verdict::Frame("Save")), - ( - "SaveAs", - Verdict::Frame("DownloadDocument"), - ), - ( - "ExportCsv", - Verdict::Frame("DownloadCsv"), - ), + ("SaveAs", Verdict::Frame("DownloadDocument")), + ("ExportCsv", Verdict::Frame("DownloadCsv")), ( "ApplyAndClearDrill", Verdict::Off("operand is one session's staged drill edits (gotcha 38, improvise-3x3u)"), @@ -134,7 +126,7 @@ fn the_verdict_list_names_only_real_effects_and_real_variants() { frames.iter().any(|f| f == variant), "{name}'s verdict names ClientFrame::{variant}, which does not exist" ), - Verdict::Off(why) | Verdict::Gap(why) => { + Verdict::Off(why) => { assert!(!why.is_empty(), "{name}'s verdict gives no argument") } } diff --git a/crates/improvise-io/src/persistence/export.rs b/crates/improvise-io/src/persistence/export.rs index 9269f6fb..44f4a68d 100644 --- a/crates/improvise-io/src/persistence/export.rs +++ b/crates/improvise-io/src/persistence/export.rs @@ -22,9 +22,8 @@ pub fn export_csv_to( storage.write(path, &render_csv(workbook, view_name)?) } -/// Render a view as CSV bytes. Separated from writing so the bytes can go to -/// any [`Storage`] backend, not just a file (improvise-fuj). Public for the -/// download frame handler (improvise-2mqz). +/// Render a view as CSV bytes for any [`Storage`] backend or a download frame +/// (improvise-fuj, improvise-2mqz). pub fn render_csv(workbook: &Workbook, view_name: &str) -> Result> { let view = workbook .views diff --git a/crates/improvise-protocol/src/frame.rs b/crates/improvise-protocol/src/frame.rs index e1a89863..29cfc183 100644 --- a/crates/improvise-protocol/src/frame.rs +++ b/crates/improvise-protocol/src/frame.rs @@ -23,13 +23,11 @@ pub enum ClientFrame { code: KeyCode, modifiers: KeyModifiers, }, - /// Ask for the document serialized as bytes. The browser downloads these - /// via Blob + ``. The filename is what the user typed in the - /// SavePrompt minibuffer — improvise-2mqz. + /// Ask for document bytes named by SavePrompt; the browser downloads them + /// via Blob + `` (improvise-2mqz). DownloadDocument { filename: String }, - /// Ask for the active view as CSV bytes. The browser downloads these - /// via Blob + ``. The filename is what the user typed in the - /// ExportPrompt minibuffer — improvise-2mqz. + /// Ask for active-view CSV bytes named by ExportPrompt; the browser downloads + /// them via Blob + `` (improvise-2mqz). DownloadCsv { filename: String }, } @@ -63,13 +61,11 @@ pub enum ServerFrame { /// The asker's mutating frame landed whole. Carries no state: everything /// it moved arrived as projections first — `context/code/protocol.md` §4. Accepted, - /// The document serialized as `.improv` format (improvise-2mqz). Addressed - /// to the asker alone; the browser turns these bytes into a download. - /// The filename is echoed from the request. + /// Downloadable `.improv` bytes addressed to the asker alone; the filename + /// is echoed from its request (improvise-2mqz). DocumentBytes { filename: String, bytes: Vec }, - /// The active view rendered as CSV (improvise-2mqz). Addressed to the - /// asker alone; the browser turns these bytes into a download. - /// The filename is echoed from the request. + /// Downloadable active-view CSV bytes addressed to the asker alone; the + /// filename is echoed from its request (improvise-2mqz). CsvBytes { filename: String, bytes: Vec }, } @@ -99,8 +95,12 @@ mod tests { )), ClientFrame::key(KeyEvent::new(KeyCode::Esc, KeyModifiers::NONE)), ClientFrame::Save, - ClientFrame::DownloadDocument { filename: "doc.improv".into() }, - ClientFrame::DownloadCsv { filename: "export.csv".into() }, + ClientFrame::DownloadDocument { + filename: "doc.improv".into(), + }, + ClientFrame::DownloadCsv { + filename: "export.csv".into(), + }, ] { let json = serde_json::to_string(&frame).expect("serializes"); assert_eq!( diff --git a/crates/improvise-protocol/src/keymap.rs b/crates/improvise-protocol/src/keymap.rs index 65788f31..4bd2e2d9 100644 --- a/crates/improvise-protocol/src/keymap.rs +++ b/crates/improvise-protocol/src/keymap.rs @@ -927,10 +927,8 @@ impl Keymaps { ), ); - // ExportPrompt and SavePrompt use rebind instead of commit because - // commit-save and commit-export return LocalThenRequest (improvise-2mqz), - // which cannot be part of a Binding::Sequence. The buffer clear is - // handled inside download_request instead. + // LocalThenRequest cannot live in a sequence, so these commands own + // their buffer clears and are rebound directly (improvise-2mqz). set.insert( ModeKey::ExportPrompt, Arc::new( diff --git a/crates/improvise-protocol/src/resolve.rs b/crates/improvise-protocol/src/resolve.rs index 6af95053..a615b5fd 100644 --- a/crates/improvise-protocol/src/resolve.rs +++ b/crates/improvise-protocol/src/resolve.rs @@ -27,10 +27,12 @@ pub enum KeyOutcome { /// Resolved here as a **request**: the caller sends the frame and folds /// nothing. Why it is not a `Command`: `context/code/clients.md`. Request(Request), - /// Fold commands locally, THEN send a request. For commit-save/commit-export: - /// the mode change folds here, the download request goes to the server. - /// (improvise-2mqz) - LocalThenRequest { cmds: Vec, request: Request }, + /// Fold commands locally, then send a request: mode changes here and + /// download bytes come from the server (improvise-2mqz). + LocalThenRequest { + cmds: Vec, + request: Request, + }, /// Bound to a command this client cannot compose. Forwarded to the host, /// which today answers nothing — `context/code/clients.md`. Upstream(KeyEvent), @@ -323,7 +325,10 @@ enum Step { Request(Request), /// Fold commands locally, THEN send a request. For commit-save/commit-export: /// the mode change folds here, the download request goes to the server. - LocalThenRequest { cmds: Vec, request: Request }, + LocalThenRequest { + cmds: Vec, + request: Request, + }, Upstream, } @@ -398,11 +403,14 @@ fn download_request(state: &ClientState, name: &str) -> Option { if filename.is_empty() { return Some(Step::Declined(vec![Command::SetStatus(format!( "No filename given — not {}", - if name == "commit-save" { "saved" } else { "exported" } + if name == "commit-save" { + "saved" + } else { + "exported" + } ))])); } - // Clear the buffer and return to Normal mode. The buffer clear is here - // rather than in the keymap sequence because LocalThenRequest cannot be + // This owns the clear and mode change because LocalThenRequest cannot be // part of a Binding::Sequence (improvise-2mqz). Some(Step::LocalThenRequest { cmds: vec![ @@ -2710,8 +2718,14 @@ mod tests { state.view.buffers.insert("item".into(), "Shirts".into()); state.view.buffers.insert("edit".into(), "42".into()); // commit-save and commit-export need their buffers filled (improvise-2mqz). - state.view.buffers.insert("save".into(), "data.improv".into()); - state.view.buffers.insert("export".into(), "data.csv".into()); + state + .view + .buffers + .insert("save".into(), "data.improv".into()); + state + .view + .buffers + .insert("export".into(), "data.csv".into()); state } @@ -2877,8 +2891,6 @@ mod tests { // Reached only from a mode `declined` refuses at the door, so a // browser is never standing in front of these. "COMMAND: Enter execute-command Silent", - // commit-export and commit-save removed: improvise-2mqz added wire - // forms (DownloadCsv, DownloadDocument), so they now ACT. // Honestly unavailable, and saying so in `declined`'s words. "NORMAL: > drill-into-cell Declines", "NORMAL: : enter-mode Declines", @@ -3533,35 +3545,37 @@ mod tests { ); } - /// The refusal above is only half the property: the bug guarded is - /// *entering a room with no exit*. See `context/test-evidence/clients.md`. - /// Note: SavePrompt and ExportPrompt are no longer refused after improvise-2mqz - /// added DownloadDocument and DownloadCsv frames. + /// A client must not enter a room with no exit. SavePrompt and ExportPrompt + /// now have download frames; see `context/test-evidence/clients.md`. #[test] fn a_prompt_this_client_cannot_commit_is_never_entered() { let state = in_mode(AppMode::Normal); - // Only CommandMode is still stranded (improvise-2mqz removed SavePrompt/ExportPrompt) - for (key, mode, subject) in [(ev(KeyCode::Char(':')), "command-mode", "command line")] { - let KeyOutcome::Local(said) = resolve(&state, key) else { - panic!("{mode} must be refused *out loud*, not in silence"); - }; - let [Command::SetStatus(msg)] = &said[..] else { - panic!("{mode}: expected one status, got {said:?}"); - }; - assert!( - msg.contains(subject), - "{mode}: the refusal must name its own subject, got {msg}" - ); - assert!( - !said.iter().any(|c| matches!(c, Command::ChangeMode(_))), - "{mode} must not be entered by a client that cannot commit it" - ); - } + let mode = "command-mode"; + let KeyOutcome::Local(said) = resolve(&state, ev(KeyCode::Char(':'))) else { + panic!("{mode} must be refused *out loud*, not in silence"); + }; + let [Command::SetStatus(msg)] = &said[..] else { + panic!("{mode}: expected one status, got {said:?}"); + }; + assert!( + msg.contains("command line"), + "{mode}: the refusal must name its own subject, got {msg}" + ); + assert!( + !said.iter().any(|c| matches!(c, Command::ChangeMode(_))), + "{mode} must not be entered by a client that cannot commit it" + ); // SavePrompt and ExportPrompt ARE now enterable (improvise-2mqz). for (key, mode_name) in [ - (KeyEvent::new(KeyCode::Char('w'), KeyModifiers::CONTROL), "save-prompt"), - (KeyEvent::new(KeyCode::Char('e'), KeyModifiers::CONTROL), "export-prompt"), + ( + KeyEvent::new(KeyCode::Char('w'), KeyModifiers::CONTROL), + "save-prompt", + ), + ( + KeyEvent::new(KeyCode::Char('e'), KeyModifiers::CONTROL), + "export-prompt", + ), ] { let outcome = resolve(&state, key); let KeyOutcome::Local(cmds) = outcome else { diff --git a/crates/improvise-wasm-client/src/socket.rs b/crates/improvise-wasm-client/src/socket.rs index b15126e4..8070d307 100644 --- a/crates/improvise-wasm-client/src/socket.rs +++ b/crates/improvise-wasm-client/src/socket.rs @@ -8,9 +8,8 @@ use improvise_protocol::{ }; use serde::{Deserialize, Serialize}; -/// A download the browser needs to trigger (improvise-2mqz). Stored after a -/// `DocumentBytes` or `CsvBytes` frame arrives; retrieved by JavaScript via -/// `Client::take_pending_download`. +/// A `DocumentBytes` or `CsvBytes` download held until JavaScript retrieves it +/// through `Client::take_pending_download` (improvise-2mqz). #[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] pub struct PendingDownload { pub filename: String, @@ -102,9 +101,8 @@ impl Client { ClientFrame::Viewport { rows, cols } } - /// Fold one server frame, in arrival order. Returns the new revision for - /// most frames; for download frames, returns the revision unchanged (no - /// state to fold) and stores the download for JavaScript to retrieve. + /// Fold one server frame in order. Downloads leave the revision unchanged + /// and wait for JavaScript; other frames return the new revision. pub fn fold_frame(&mut self, frame: ServerFrame) -> u64 { match frame { ServerFrame::Snapshot(snapshot) => self.bootstrap(*snapshot), diff --git a/crates/improvise-worker-server/tests/post_message.rs b/crates/improvise-worker-server/tests/post_message.rs index 44ebfa65..288570b4 100644 --- a/crates/improvise-worker-server/tests/post_message.rs +++ b/crates/improvise-worker-server/tests/post_message.rs @@ -353,11 +353,17 @@ fn the_shipped_demo_seeds_a_worker_whose_raw_cells_are_settable() { } // The third column is `Profit`, which a formula owns: the page must not // offer it, so it carries no surrogate to offer (improvise-ir7l). - for row in 0..main.grid.row_frame.len() { + for (row, cells) in main + .grid + .cells + .iter() + .enumerate() + .take(main.grid.row_frame.len()) + { assert!( - matches!(main.grid.cells[row][2].id, Some(CellId::Derived { .. })), + matches!(cells[2].id, Some(CellId::Derived { .. })), "row {row}'s Profit cell must be Derived, got {:?}", - main.grid.cells[row][2].id + cells[2].id ); } diff --git a/web/tests/bundle/wasm_surface.test.js b/web/tests/bundle/wasm_surface.test.js index a9f8550f..7f5e9f0a 100644 --- a/web/tests/bundle/wasm_surface.test.js +++ b/web/tests/bundle/wasm_surface.test.js @@ -91,6 +91,9 @@ test("the page still reaches the client through a handful of calls", () => { "on_key_event", "render_model", "set_viewport", + // improvise-2mqz: download frames stay in the Rust fold until the page + // takes the bytes and hands them to Blob + . + "take_pending_download", "typed_cell", ], );