From 0c2d46662dd4a101c639765dcc42f2cbc3c223fa Mon Sep 17 00:00:00 2001 From: Shunichiro Nomura Date: Wed, 14 Oct 2026 20:45:02 +0900 Subject: [PATCH 1/8] view: project the view only for its viewers, in linear time The language server projected a view model on every analysis and the browser engine on every prepareProject, the playground's included, though only a view reads it; the projection also rebuilt a file's line index per declaration, quadratic in the file (4,000 declarations: +0.9 s per analysis in release). Analysis now projects only for a client that declares experimental.graphcal.viewModelChanges (ViewModelDemand), the engine only through the new prepareViewProject, which web/view uses, and a prepared project without a view says so instead of "has no view". Each file's line index is built once. Co-Authored-By: Claude Opus 5.5 --- crates/graphcal-lsp/src/analysis.rs | 3 +- crates/graphcal-lsp/src/analysis_pipeline.rs | 58 ++++++- crates/graphcal-lsp/src/server.rs | 47 +++--- .../src/view_model/project.rs | 25 +-- crates/graphcal-wasm/src/lib.rs | 2 +- crates/graphcal-wasm/src/prepared.rs | 145 +++++++++++++++--- web/view/src/engine.ts | 5 +- 7 files changed, 223 insertions(+), 62 deletions(-) diff --git a/crates/graphcal-lsp/src/analysis.rs b/crates/graphcal-lsp/src/analysis.rs index bb6f33672..33f2561a6 100644 --- a/crates/graphcal-lsp/src/analysis.rs +++ b/crates/graphcal-lsp/src/analysis.rs @@ -59,7 +59,8 @@ pub struct AnalysisResult { pub eval_values: HashMap, /// The document's view model (structure, and values when it evaluated), /// served to the `graphcal/viewModel` request. `None` when the project - /// did not check. + /// did not check, or when the client shows no view models (the analysis + /// then skips the projection). pub view: Option, /// Structured function signatures, keyed by function name. /// Points to a lazily-initialized static map (builtins never change). diff --git a/crates/graphcal-lsp/src/analysis_pipeline.rs b/crates/graphcal-lsp/src/analysis_pipeline.rs index cf337dc77..2e18c6afb 100644 --- a/crates/graphcal-lsp/src/analysis_pipeline.rs +++ b/crates/graphcal-lsp/src/analysis_pipeline.rs @@ -60,6 +60,18 @@ impl AnalysisDegradation { } } +/// Whether an analysis also projects the document's view model. +/// +/// The view is experimental: only a client that shows view models +/// (`experimental.graphcal.viewModelChanges`) pays for the projection. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub enum ViewModelDemand { + /// No view is shown: the analysis carries no view model. + Skip, + /// The client shows view models: project one after checking. + Project, +} + /// Transient output of one analysis pass. pub struct AnalysisRun { pub analysis: AnalysisResult, @@ -258,6 +270,7 @@ pub(crate) fn run_analysis_run( open_buffers, plugin_host, &input_snapshot, + ViewModelDemand::Project, &graphcal_compiler::cancellation::CancellationToken::unbounded(), ) .expect("an unbounded analysis cannot be cancelled") @@ -287,6 +300,7 @@ pub fn run_analysis_with_cancellation( open_buffers: &[OpenBuffer], plugin_host: &graphcal_plugin_host::PluginHost, input_snapshot: &AnalysisInputSnapshot, + view: ViewModelDemand, cancellation: &CancellationToken, ) -> std::result::Result { cancellation.checkpoint()?; @@ -396,14 +410,17 @@ pub fn run_analysis_with_cancellation( // user opens such a file for editing. let todos = crate::diagnostics::unfinished_node_diagnostics(&root_ast.declarations, text); - let (mut view, degradations) = match project_view_for_lsp(&project, &checked) { - Ok(view) => (Some(view), Vec::new()), - Err(error) => ( - None, - vec![AnalysisDegradation::ViewUnavailable { - reason: error.to_string(), - }], - ), + let (mut view, degradations) = match view { + ViewModelDemand::Skip => (None, Vec::new()), + ViewModelDemand::Project => match project_view_for_lsp(&project, &checked) { + Ok(view) => (Some(view), Vec::new()), + Err(error) => ( + None, + vec![AnalysisDegradation::ViewUnavailable { + reason: error.to_string(), + }], + ), + }, }; cancellation.checkpoint()?; let (mut diagnostics, eval_values) = if checked.is_library() { @@ -625,4 +642,29 @@ mod view_tests { assert_eq!(names, ["thrust", "doubled"]); assert!(view.nodes[1].value.is_some(), "doubled should be evaluated"); } + + #[test] + fn analysis_skips_the_view_model_unless_asked() { + let uri = Url::parse("untitled:view.gcl").unwrap(); + let input_snapshot = AnalysisInputSnapshot::new( + crate::file_identity::document_identity(&uri), + crate::workspace_revision::RevisionClock::default() + .next() + .unwrap(), + HashMap::new(), + ); + let run = run_analysis_with_cancellation( + &uri, + "node thrust: Dimensionless = 1.0;\n", + &[], + test_plugin_host(), + &input_snapshot, + ViewModelDemand::Skip, + &CancellationToken::unbounded(), + ) + .unwrap(); + assert_eq!(run.degradations, Vec::::new()); + assert!(run.analysis.view.is_none()); + assert!(run.analysis.has_no_diagnostics()); + } } diff --git a/crates/graphcal-lsp/src/server.rs b/crates/graphcal-lsp/src/server.rs index 9acfbf92c..a954f39ee 100644 --- a/crates/graphcal-lsp/src/server.rs +++ b/crates/graphcal-lsp/src/server.rs @@ -24,7 +24,9 @@ use tower_lsp::lsp_types::{ use tower_lsp::{Client, ClientSocket, LanguageServer, LspService, Server}; use crate::analysis::AnalysisResult; -use crate::analysis_pipeline::{AnalysisRun, OpenBuffer, run_analysis_with_cancellation}; +use crate::analysis_pipeline::{ + AnalysisRun, OpenBuffer, ViewModelDemand, run_analysis_with_cancellation, +}; use crate::analysis_schedule_state::AnalysisScheduleState; use crate::client_capabilities::{ClientFeatureSupport, DocumentSymbolShape, WorkspaceEditShape}; use crate::convert::position_to_byte_offset; @@ -730,6 +732,11 @@ async fn analyze_store_publish_once( } let uri_for_analysis = uri.clone(); + let view = if client_features.view_model_changes.is_supported() { + ViewModelDemand::Project + } else { + ViewModelDemand::Skip + }; let source = registration.source; let worker_plugin_host = Arc::clone(plugin_host); let mut task = tokio::task::spawn_blocking(move || { @@ -743,6 +750,7 @@ async fn analyze_store_publish_once( &open_buffers, &worker_plugin_host, &input_snapshot, + view, &cancellation, ) }); @@ -1494,8 +1502,9 @@ pub(crate) fn service() -> (LspService, ClientSocket) { } /// `graphcal/viewModel`: the view model of a document, as of its latest -/// analysis. `null` until the document has been analyzed, or when its -/// project did not check. +/// analysis. `null` until the document has been analyzed, when its project +/// did not check, or when the client did not declare +/// `experimental.graphcal.viewModelChanges` (analysis then skips the view). enum ViewModelRequest {} impl ViewModelRequest { @@ -3510,20 +3519,21 @@ node momentum: Force * Time = @mass * @velocity; root_revision, HashMap::from([(dependency.clone(), old_dependency_revision)]), ); + let analyze = |overlay: &[OpenBuffer], input: &AnalysisInputSnapshot| { + let unbounded = CancellationToken::unbounded(); + let host = test_plugin_host(); + let view = ViewModelDemand::Skip; + run_analysis_with_cancellation( + &main_uri, &main_text, overlay, host, input, view, &unbounded, + ) + .unwrap() + .analysis + }; let overlay = vec![OpenBuffer { path: lib_path, text: Arc::new("pub const node y: Dimensionless = 2.0;\n".to_string()), }]; - let analysis = run_analysis_with_cancellation( - &main_uri, - &main_text, - &overlay, - test_plugin_host(), - &input, - &CancellationToken::unbounded(), - ) - .unwrap() - .analysis; + let analysis = analyze(&overlay, &input); assert!(analysis.has_no_diagnostics()); assert!( analysis @@ -3549,16 +3559,7 @@ node momentum: Force * Time = @mass * @velocity; path: dir.path().join("src/fresh/lib.gcl"), text: Arc::new("pub const node q: Dimensionless = 4.0;\n".to_string()), }]; - let refreshed = run_analysis_with_cancellation( - &main_uri, - &main_text, - &refreshed_overlay, - test_plugin_host(), - &refreshed_input, - &CancellationToken::unbounded(), - ) - .unwrap() - .analysis; + let refreshed = analyze(&refreshed_overlay, &refreshed_input); assert!(!refreshed.has_no_diagnostics()); let use_offset = main_text.rfind('y').unwrap(); assert!(goto_definition::goto_definition(&analysis, &main_uri, use_offset).is_some()); diff --git a/crates/graphcal-project/src/view_model/project.rs b/crates/graphcal-project/src/view_model/project.rs index 208f0898e..06b70661e 100644 --- a/crates/graphcal-project/src/view_model/project.rs +++ b/crates/graphcal-project/src/view_model/project.rs @@ -626,13 +626,10 @@ impl NodeBuilder<'_> { let expr = input .file .and_then(|file| definition.and_then(|span| slice(file.text, span))); - let source = input.file.map(|file| { - let index = Utf16LineIndex::new(file.text); - ViewSource { - file: Some(file.name.to_string()), - line: index.position(span.offset()).line.saturating_add(1), - expr_range: definition.map(|span| index.range(span.offset(), span.len()).into()), - } + let source = input.file.map(|file| ViewSource { + file: Some(file.name.to_string()), + line: file.lines.position(span.offset()).line.saturating_add(1), + expr_range: definition.map(|span| file.lines.range(span.offset(), span.len()).into()), }); let binding = ValueBinding { @@ -759,6 +756,8 @@ impl<'e> EvaluatedValues<'e> { struct SourceFiles<'p> { project: &'p LoadedProject, names: HashMap<&'p DagId, String>, + /// Each file's line index: one scan per file, not one per declaration. + lines: HashMap<&'p DagId, Utf16LineIndex<'p>>, } impl<'p> SourceFiles<'p> { @@ -768,6 +767,7 @@ impl<'p> SourceFiles<'p> { root_dir: Option<&Path>, ) -> Result { let mut names = HashMap::new(); + let mut lines = HashMap::new(); for file in project.files().iter() { // The name is a project-relative path with `/` on every platform: it is // the wire format the view and the editors key their files by. @@ -788,8 +788,13 @@ impl<'p> SourceFiles<'p> { .to_string(), }; names.insert(file.dag_id(), name); + lines.insert(file.dag_id(), Utf16LineIndex::new(file.source().as_str())); } - Ok(Self { project, names }) + Ok(Self { + project, + names, + lines, + }) } fn file_of(&self, module: &DagId) -> Option> { @@ -797,6 +802,7 @@ impl<'p> SourceFiles<'p> { Some(SourceFile { name: self.names.get(file.dag_id())?.as_str(), text: file.source().as_str(), + lines: self.lines.get(file.dag_id())?, }) } @@ -805,11 +811,12 @@ impl<'p> SourceFiles<'p> { } } -/// One source file: its view name and text. +/// One source file: its view name, text, and line index. #[derive(Clone, Copy)] struct SourceFile<'p> { name: &'p str, text: &'p str, + lines: &'p Utf16LineIndex<'p>, } /// The unit an unevaluated quantity declaration would be shown in. diff --git a/crates/graphcal-wasm/src/lib.rs b/crates/graphcal-wasm/src/lib.rs index cd278d817..73e892c55 100644 --- a/crates/graphcal-wasm/src/lib.rs +++ b/crates/graphcal-wasm/src/lib.rs @@ -32,7 +32,7 @@ pub use output::{ }; pub use prepared::{ BindingErrorView, ControlView, EvaluateOutcome, EvaluateReportOutcome, ParameterPortView, - PrepareOutcome, PreparedPlayground, prepare, prepare_bundle, + PrepareOutcome, PreparedPlayground, prepare, prepare_bundle, prepare_view, }; pub use project::{ MAX_PLAYGROUND_CONTENT_BYTES, MAX_PLAYGROUND_FILE_BYTES, MAX_PLAYGROUND_FILES, diff --git a/crates/graphcal-wasm/src/prepared.rs b/crates/graphcal-wasm/src/prepared.rs index f8d3766a6..47b627171 100644 --- a/crates/graphcal-wasm/src/prepared.rs +++ b/crates/graphcal-wasm/src/prepared.rs @@ -36,9 +36,26 @@ use crate::project::{ProjectValidationError, RequestErrorView, VirtualProject}; pub struct PreparedPlayground { prepared: PreparedProject, report: crate::browser_report::ReportMetadata, - /// The entry file's view structure, valued per evaluation; `None` when - /// the checked program could not be projected. - view: Option, + /// The entry file's view structure, valued per evaluation. + view: PreparedView, +} + +/// The entry file's view, as far as preparing produced one. +enum PreparedView { + /// Prepared for evaluation alone ([`prepare`]): no view was asked for. + NotRequested, + /// The checked program could not be projected. + Unavailable { reason: String }, + /// The structure, valued per evaluation. + Projected(ViewProjection), +} + +/// Whether preparing also projects the entry file's view model. The view is +/// experimental: only the view page pays for the projection. +#[derive(Clone, Copy)] +enum ViewDemand { + Skip, + Project, } /// Outcome of preparing one browser project. @@ -67,14 +84,39 @@ pub fn prepare(request: PlaygroundRequest) -> PrepareOutcome { } }; - prepare_virtual(&project, BrowserCapabilities::SourcesOnly) + prepare_virtual(&project, BrowserCapabilities::SourcesOnly, ViewDemand::Skip) +} + +/// [`prepare`], also projecting the entry file's view model for +/// [`PreparedPlayground::view_structure`] and +/// [`PreparedPlayground::evaluate_view`]. +#[must_use] +pub fn prepare_view(request: PlaygroundRequest) -> PrepareOutcome { + let project = match VirtualProject::try_from(request) { + Ok(project) => project, + Err(error) => { + return PrepareOutcome::Rejected { + error: RequestErrorView::from(&error), + }; + } + }; + + prepare_virtual( + &project, + BrowserCapabilities::SourcesOnly, + ViewDemand::Project, + ) } /// Prepare an offline report snapshot, including sandboxed Wasm plugins. #[must_use] pub fn prepare_bundle(bundle: &graphcal_project::project_bundle::ProjectBundle) -> PrepareOutcome { match VirtualProject::from_bundle(bundle) { - Ok(project) => prepare_virtual(&project, BrowserCapabilities::BundledPlugins), + Ok(project) => prepare_virtual( + &project, + BrowserCapabilities::BundledPlugins, + ViewDemand::Skip, + ), Err(error) => PrepareOutcome::Rejected { error: RequestErrorView::from(&ProjectValidationError::InvalidBundle { message: error.to_string(), @@ -93,7 +135,11 @@ enum BrowserCapabilities { clippy::result_large_err, reason = "the loader reports its compile error by value, as every loader entry point does" )] -fn prepare_virtual(project: &VirtualProject, capabilities: BrowserCapabilities) -> PrepareOutcome { +fn prepare_virtual( + project: &VirtualProject, + capabilities: BrowserCapabilities, + view: ViewDemand, +) -> PrepareOutcome { let filesystem = project.filesystem(); let loaded = match graphcal_compiler::outcome::without_cancellation(|cancellation| { load_project_with_dependency_sources( @@ -134,7 +180,17 @@ fn prepare_virtual(project: &VirtualProject, capabilities: BrowserCapabilities) .host_fns(®istry) .check() .and_then(|checked| { - let view = project_view(&loaded, &checked, Some(VirtualProject::root_path())).ok(); + let view = match view { + ViewDemand::Skip => PreparedView::NotRequested, + ViewDemand::Project => { + match project_view(&loaded, &checked, Some(VirtualProject::root_path())) { + Ok(projection) => PreparedView::Projected(projection), + Err(error) => PreparedView::Unavailable { + reason: error.to_string(), + }, + } + } + }; checked .prepare_with_host_fns(®istry) .map(|prepared| (prepared, view)) @@ -354,20 +410,32 @@ impl PreparedPlayground { } } - /// The entry file's view structure without values, when it projected. + /// The entry file's view structure without values, when it was asked + /// for ([`prepare_view`]) and projected. #[must_use] - pub fn view_structure(&self) -> Option<&ViewModel> { - self.view.as_ref().map(ViewProjection::model) + pub const fn view_structure(&self) -> Option<&ViewModel> { + match &self.view { + PreparedView::Projected(projection) => Some(projection.model()), + PreparedView::NotRequested | PreparedView::Unavailable { .. } => None, + } } /// Evaluate browser bindings and return the entry file's view with the /// resulting values attached. #[must_use] pub fn evaluate_view(&self, bindings: &[BrowserBindingRequest]) -> EvaluateViewOutcome { - let Some(projection) = &self.view else { - return EvaluateViewOutcome::Unavailable { - message: "the project has no view".to_string(), - }; + let projection = match &self.view { + PreparedView::Projected(projection) => projection, + PreparedView::NotRequested => { + return EvaluateViewOutcome::Unavailable { + message: "the project was prepared without its view".to_string(), + }; + } + PreparedView::Unavailable { reason } => { + return EvaluateViewOutcome::Unavailable { + message: reason.clone(), + }; + } }; match self.evaluate_browser_result(bindings) { Ok(result) => { @@ -810,10 +878,28 @@ mod js { reason = "wasm-bindgen JavaScript exports receive owned JsValue handles" )] pub fn prepare_project_js(request: JsValue) -> Result { + prepare_request_js(&request, super::prepare) + } + + /// `prepareProject`, also projecting the entry file's view model for + /// `viewStructure` and `evaluateView`. Only the view page pays for it. + #[wasm_bindgen(js_name = prepareViewProject)] + #[expect( + clippy::needless_pass_by_value, + reason = "wasm-bindgen JavaScript exports receive owned JsValue handles" + )] + pub fn prepare_view_project_js(request: JsValue) -> Result { + prepare_request_js(&request, super::prepare_view) + } + + fn prepare_request_js( + request: &JsValue, + prepare: fn(super::PlaygroundRequest) -> PrepareOutcome, + ) -> Result { use crate::js_request::{BoundedJsRequest, JsRequestBoundaryError}; - let outcome = match BoundedJsRequest::decode(&request) { - Ok(request) => super::prepare(request.into_request()), + let outcome = match BoundedJsRequest::decode(request) { + Ok(request) => prepare(request.into_request()), Err(JsRequestBoundaryError::Rejected(error)) => PrepareOutcome::Rejected { error: crate::RequestErrorView::from(&error), }, @@ -993,7 +1079,15 @@ mod tests { } fn prepare_ok(source: &str) -> PreparedPlayground { - match prepare(single_file(source)) { + prepared_or_panic(prepare(single_file(source))) + } + + fn prepare_view_ok(source: &str) -> PreparedPlayground { + prepared_or_panic(prepare_view(single_file(source))) + } + + fn prepared_or_panic(outcome: PrepareOutcome) -> PreparedPlayground { + match outcome { PrepareOutcome::Prepared(prepared) => *prepared, PrepareOutcome::Rejected { error } => panic!("rejected: {error:?}"), PrepareOutcome::CompileError { diagnostics } => { @@ -1042,8 +1136,23 @@ assert positive = @delta_v > 0.0 m/s; } #[test] - fn view_structure_and_evaluated_view_follow_bindings() { + fn playground_preparation_projects_no_view() { let prepared = prepare_ok(DELTA_V); + assert!(prepared.view_structure().is_none()); + let outcome = prepared.evaluate_view(&[]); + assert!( + matches!( + &outcome, + EvaluateViewOutcome::Unavailable { message } + if message == "the project was prepared without its view" + ), + "{outcome:?}" + ); + } + + #[test] + fn view_structure_and_evaluated_view_follow_bindings() { + let prepared = prepare_view_ok(DELTA_V); let structure = prepared.view_structure().expect("view structure"); assert!(structure.nodes.iter().all(|node| node.value.is_none())); assert!(structure.nodes.iter().any(|node| node.name == "delta_v")); diff --git a/web/view/src/engine.ts b/web/view/src/engine.ts index 917573c25..c46aee53b 100644 --- a/web/view/src/engine.ts +++ b/web/view/src/engine.ts @@ -112,7 +112,8 @@ interface Prepared { } interface Engine { default(options: { module_or_path: ArrayBuffer }): Promise; - prepareProject(request: { entry: string; files: ProjectFile[] }): Prepared; + /** `prepareProject` that also projects the entry file's view (the playground skips it). */ + prepareViewProject(request: { entry: string; files: ProjectFile[] }): Prepared; } let loading: Promise | null = null; @@ -141,7 +142,7 @@ export function loadEngine(): Promise { async function prepare(entry: string, files: ProjectFile[]): Promise { const engine = await loadEngine(); try { - return engine.prepareProject({ entry, files }); + return engine.prepareViewProject({ entry, files }); } catch (error) { const failure = outcomeSchema.safeParse(error); if (failure.success) return failure.data; From 7b5b17ff928eeea4f300308f3e25958c62e16fee Mon Sep 17 00:00:00 2001 From: Shunichiro Nomura Date: Wed, 14 Oct 2026 20:48:45 +0900 Subject: [PATCH 2/8] web/view: VS Code edits name their copy and wait for the extension's answer The webview keeps each source copy by project and revision, sends both with an edit, and takes the edit into its copy only once the extension answers that it applied it; a refused edit leaves the copy as it was and shows why. Two projects' `src/main.gcl` no longer share one copy. Pairs with vscode-graphcal's address-reviews branch, whose extension checks the revision against the document before applying an edit. Co-Authored-By: Claude Opus 5.5 --- web/view/src/vscodeHost.test.ts | 115 ++++++++++++++++++++++++++++++++ web/view/src/vscodeHost.ts | 104 ++++++++++++++++++++++++----- 2 files changed, 202 insertions(+), 17 deletions(-) create mode 100644 web/view/src/vscodeHost.test.ts diff --git a/web/view/src/vscodeHost.test.ts b/web/view/src/vscodeHost.test.ts new file mode 100644 index 000000000..9d269ac39 --- /dev/null +++ b/web/view/src/vscodeHost.test.ts @@ -0,0 +1,115 @@ +import { afterEach, beforeEach, describe, expect, test, vi } from "vite-plus/test"; +import type { ViewModel } from "./model.ts"; +import { minimalEdit, VsCodeHost } from "./vscodeHost.ts"; + +let sent: { type: string; [key: string]: unknown }[] = []; +let deliver: (message: unknown) => void = () => {}; + +beforeEach(() => { + sent = []; + vi.stubGlobal("acquireVsCodeApi", () => ({ + postMessage: (message: { type: string }) => sent.push(message), + })); + vi.stubGlobal("window", { + addEventListener: (_: string, listener: (event: { data: unknown }) => void) => { + deliver = (message) => listener({ data: message }); + }, + }); +}); + +afterEach(() => { + vi.unstubAllGlobals(); +}); + +function modelOf(project: string, file: string, text: string, revision: number): void { + deliver({ + type: "model", + model: { + version: 0, + module: { id: `${project}.main`, file, package: project }, + indexes: [], + nodes: [], + }, + project: `file:///work/${project}`, + sources: { [file]: text }, + revision, + main: true, + }); +} + +function shown(host: VsCodeHost, project: string): ViewModel { + const model = host.model(`${project}.main`); + if (model === null) throw new Error("no model"); + return model; +} + +function lastEdit(): { type: string; [key: string]: unknown } { + const edit = sent.findLast((m) => m.type === "edit"); + if (edit === undefined) throw new Error("no edit sent"); + return edit; +} + +describe("VsCodeHost", () => { + test("names the copy's project and revision, and waits for the answer to take the edit", async () => { + const host = new VsCodeHost(); + modelOf("a", "src/main.gcl", "param n = 1;\n", 3); + const model = shown(host, "a"); + let settled = false; + const result = host.edit(model, { file: "src/main.gcl", next: "param n = 2;\n" }); + void result.then(() => (settled = true)); + const edit = lastEdit(); + expect(edit).toMatchObject({ + project: "file:///work/a", + file: "src/main.gcl", + revision: 3, + start: 10, + end: 11, + text: "2", + }); + await Promise.resolve(); + expect(settled).toBe(false); + expect(host.source(model, "src/main.gcl")).toBe("param n = 1;\n"); + deliver({ type: "edited", id: edit.id, ok: true }); + expect(await result).toEqual({ ok: true, models: [] }); + expect(host.source(model, "src/main.gcl")).toBe("param n = 2;\n"); + }); + + test("keeps its copy when the extension refuses the edit", async () => { + const host = new VsCodeHost(); + modelOf("a", "src/main.gcl", "param n = 1;\n", 3); + const model = shown(host, "a"); + const result = host.edit(model, { file: "src/main.gcl", next: "param n = 2;\n" }); + deliver({ type: "edited", id: lastEdit().id, ok: false, message: "changed" }); + expect(await result).toEqual({ ok: false, message: "changed" }); + expect(host.source(model, "src/main.gcl")).toBe("param n = 1;\n"); + }); + + test("keeps the same file of two projects apart", () => { + const host = new VsCodeHost(); + modelOf("a", "src/main.gcl", "param n = 1;\n", 1); + modelOf("b", "src/main.gcl", "param m = 5;\n", 2); + expect(host.source(shown(host, "a"), "src/main.gcl")).toBe("param n = 1;\n"); + expect(host.source(shown(host, "b"), "src/main.gcl")).toBe("param m = 5;\n"); + void host.edit(shown(host, "a"), { file: "src/main.gcl", next: "param n = 2;\n" }); + expect(lastEdit()).toMatchObject({ project: "file:///work/a", revision: 1 }); + }); + + test("refuses to edit a file it holds no copy of", async () => { + const host = new VsCodeHost(); + modelOf("a", "src/main.gcl", "param n = 1;\n", 1); + expect(await host.edit(shown(host, "a"), { file: "src/other.gcl", next: "" })).toEqual({ + ok: false, + message: "unknown file src/other.gcl", + }); + }); +}); + +describe("minimalEdit", () => { + test("trims the shared prefix and suffix", () => { + expect(minimalEdit("param n = 1;\n", "param n = 2;\n")).toEqual({ + start: 10, + end: 11, + text: "2", + }); + }); +}); diff --git a/web/view/src/vscodeHost.ts b/web/view/src/vscodeHost.ts index 01f2e0ffb..08ba89235 100644 --- a/web/view/src/vscodeHost.ts +++ b/web/view/src/vscodeHost.ts @@ -1,20 +1,42 @@ // The VS Code webview's host: the extension sends models (projected by the -// language server) and the sources they came from; edits go back as text -// replacements on the editor's documents. The server re-analyzes the changed -// document and the extension sends the fresh model, so an edit resolves at -// once and the view updates when the model arrives. +// language server) and copies of the sources they came from; edits go back as +// text replacements on those copies. Each copy comes with its project and a +// revision, and an edit names both, so the extension applies it to the +// document the copy was read from, and only while that copy is current. The +// extension answers every edit; the copy takes the edit once it is applied, +// and the view updates when the server's fresh model arrives. import type { EditResult, Host, NoteKind, SourceEdit } from "./host.ts"; import { parseViewModel, type ViewModel } from "./model.ts"; /** Messages from the extension. */ type Inbound = - | { type: "model"; model: unknown; sources: Record; main: boolean } + | { + type: "model"; + model: unknown; + /** The project root the model's file paths are relative to. */ + project: string; + sources: Record; + /** The revision of these copies, which an edit on one names. */ + revision: number; + main: boolean; + } + | { type: "edited"; id: number; ok: true } + | { type: "edited"; id: number; ok: false; message: string } | { type: "note"; message: string | null }; /** Messages to the extension. */ type Outbound = | { type: "ready" } - | { type: "edit"; file: string; start: number; end: number; text: string } + | { + type: "edit"; + id: number; + project: string; + file: string; + revision: number; + start: number; + end: number; + text: string; + } | { type: "request"; module: string }; interface VsCodeApi { @@ -45,12 +67,28 @@ export function minimalEdit( return { start, end: endBefore, text: after.slice(start, endAfter) }; } +/** A file as this webview holds it, and the revision of the copy it started from. */ +interface Copy { + text: string; + revision: number; +} + +/** The key of a copy: two projects may both have `src/main.gcl`. */ +function copyKey(project: string, file: string): string { + return JSON.stringify([project, file]); +} + export class VsCodeHost implements Host { readonly modules: readonly string[] = []; readonly edited = false; private readonly api = acquireVsCodeApi(); private readonly models = new Map(); - private readonly sources = new Map(); + /** The project each model came from; its file paths are relative to that root. */ + private readonly projects = new WeakMap(); + private readonly copies = new Map(); + /** Edits sent and not answered yet, by id. */ + private readonly pending = new Map void>(); + private edits = 0; private readonly modelListeners: ((models: ViewModel[]) => void)[] = []; private readonly openListeners: ((module: string) => void)[] = []; private readonly noteListeners: ((message: string | null, kind: NoteKind) => void)[] = []; @@ -73,7 +111,10 @@ export class VsCodeHost implements Host { return; } this.models.set(model.module.id, model); - for (const [file, text] of Object.entries(message.sources)) this.sources.set(file, text); + this.projects.set(model, message.project); + for (const [file, text] of Object.entries(message.sources)) { + this.copies.set(copyKey(message.project, file), { text, revision: message.revision }); + } if (message.main && this.mainModule !== model.module.id) { this.mainModule = model.module.id; for (const listener of this.openListeners) listener(model.module.id); @@ -82,6 +123,12 @@ export class VsCodeHost implements Host { } return; } + case "edited": { + const settle = this.pending.get(message.id); + this.pending.delete(message.id); + settle?.(message.ok ? { ok: true, models: [] } : { ok: false, message: message.message }); + return; + } case "note": this.note(message.message); return; @@ -98,18 +145,41 @@ export class VsCodeHost implements Host { return this.models.get(module) ?? null; } - source(_model: ViewModel, file: string): string | null { - return this.sources.get(file) ?? null; + source(model: ViewModel, file: string): string | null { + const project = this.projects.get(model); + if (project === undefined) return null; + return this.copies.get(copyKey(project, file))?.text ?? null; } - edit(_model: ViewModel, edit: SourceEdit): Promise { - const before = this.sources.get(edit.file); - if (before === undefined) + edit(model: ViewModel, edit: SourceEdit): Promise { + const project = this.projects.get(model); + const key = project === undefined ? null : copyKey(project, edit.file); + const copy = key === null ? undefined : this.copies.get(key); + if (project === undefined || key === null || copy === undefined) { return Promise.resolve({ ok: false, message: `unknown file ${edit.file}` }); - const { start, end, text } = minimalEdit(before, edit.next); - this.sources.set(edit.file, edit.next); - this.api.postMessage({ type: "edit", file: edit.file, start, end, text }); - return Promise.resolve({ ok: true, models: [] }); + } + const { start, end, text } = minimalEdit(copy.text, edit.next); + this.edits += 1; + const id = this.edits; + return new Promise((resolve) => { + this.pending.set(id, (result) => { + // Applied: the copy is the edited text, unless a fresh copy came first. + if (result.ok && this.copies.get(key) === copy) { + this.copies.set(key, { text: edit.next, revision: copy.revision }); + } + resolve(result); + }); + this.api.postMessage({ + type: "edit", + id, + project, + file: edit.file, + revision: copy.revision, + start, + end, + text, + }); + }); } request(module: string): void { From bd2d5a921e82a51249731338dcd7a0d3fcd11e6f Mon Sep 17 00:00:00 2001 From: Shunichiro Nomura Date: Wed, 14 Oct 2026 20:49:46 +0900 Subject: [PATCH 3/8] view: an inline dag's declarations stay out of the sheet The projection took every source module of the root file as local, the inline dag templates included: their params and nodes became valueless root nodes whose leaf-name ids collided with the root's own (two dags' `out`, or a root `out` beside a template's), so the sheet's id-keyed maps dropped one and wired readers to the wrong declaration. Only the root DAG is local now; a template is read through its instances, which the view already shows as externals. The projection checks that every node id is unique and every dependency names a node, and fails instead of returning a model the view would miswire. Co-Authored-By: Claude Opus 5.5 --- .../src/view_model/project.rs | 55 ++++++--- .../graphcal-project/src/view_model/tests.rs | 114 ++++++++++++++++++ 2 files changed, 155 insertions(+), 14 deletions(-) diff --git a/crates/graphcal-project/src/view_model/project.rs b/crates/graphcal-project/src/view_model/project.rs index 06b70661e..a6150ef16 100644 --- a/crates/graphcal-project/src/view_model/project.rs +++ b/crates/graphcal-project/src/view_model/project.rs @@ -1,7 +1,7 @@ //! Projection of a checked project (and optionally its evaluation) into a //! [`ViewModel`]. -use std::collections::{BTreeMap, BTreeSet, HashMap}; +use std::collections::{BTreeMap, BTreeSet, HashMap, HashSet}; use std::path::Path; use graphcal_compiler::dag_id::DagId; @@ -57,6 +57,12 @@ pub enum ViewProjectionError { #[source] error: DisplayProjectionError, }, + /// Two nodes of the model would share one id. + #[error("two view nodes share the id `{id}`")] + DuplicateNodeId { id: String }, + /// A node reads a node the model does not contain. + #[error("view node `{node}` reads `{dependency}`, which the view does not contain")] + UnknownDependency { node: String, dependency: String }, } /// Project the structure of the root file of `checked`. @@ -120,6 +126,7 @@ pub fn project_view( } } let indexes = builder.indexes(); + check_node_references(&nodes)?; let root = tir.root_dag_id(); Ok(ViewProjection { @@ -247,12 +254,11 @@ impl<'p> Scope<'p> { self.tir.root_dag_id() } - /// Whether `owner` is the root file or an inline DAG declared in it. + /// Whether `owner` is the root DAG. An inline `dag` declared in the + /// root file is a template: its declarations hold no values of their + /// own, and the root reads them only through an instance. fn is_local(&self, owner: &DagId) -> bool { - matches!( - self.clusters.get(owner), - Some(GraphClusterKind::SourceModule) - ) && owner.file_root() == self.root().file_root() + owner == self.root() } /// The module a declaration is presented under: the template of an @@ -303,17 +309,15 @@ impl<'p> Scope<'p> { }) } - /// The root's value and assertion declarations, with those of its inline - /// DAGs, in source order. (An include alias is declared where the - /// `include` is written, not where the DAG lists it.) + /// The root's value and assertion declarations in source order. (An + /// include alias is declared where the `include` is written, not where + /// the DAG lists it.) fn local_declarations(&self) -> Vec { let mut declarations: Vec<(usize, ResolvedDeclName)> = self - .clusters - .keys() - .filter(|owner| self.is_local(owner)) - .collect::>() + .tir + .dag_registry() + .get(self.root()) .into_iter() - .filter_map(|owner| self.tir.dag_registry().get(owner)) .flat_map(|dag| { dag.declarations().filter_map(|declaration| { let span = match declaration.kind() { @@ -696,6 +700,29 @@ struct NodeInput<'a> { value_of: &'a ResolvedDeclName, } +/// Every node id is unique, and every dependency names a node of the model: +/// the view joins nodes by id, so a clash or a dangling id would wire the +/// wrong declarations together. +pub(super) fn check_node_references(nodes: &[ViewNode]) -> Result<(), ViewProjectionError> { + let mut ids = HashSet::with_capacity(nodes.len()); + for node in nodes { + if !ids.insert(node.id.as_str()) { + return Err(ViewProjectionError::DuplicateNodeId { + id: node.id.clone(), + }); + } + } + for node in nodes { + if let Some(dependency) = node.deps.iter().find(|dep| !ids.contains(dep.as_str())) { + return Err(ViewProjectionError::UnknownDependency { + node: node.id.clone(), + dependency: dependency.clone(), + }); + } + } + Ok(()) +} + fn declaration_of<'t>( tir: &'t CheckedTir, id: &ResolvedDeclName, diff --git a/crates/graphcal-project/src/view_model/tests.rs b/crates/graphcal-project/src/view_model/tests.rs index 652412ff9..710a09886 100644 --- a/crates/graphcal-project/src/view_model/tests.rs +++ b/crates/graphcal-project/src/view_model/tests.rs @@ -183,3 +183,117 @@ fn single_file_project_projects() { assert_eq!(view.nodes.len(), 2); assert_eq!(node(&view, "doubled").deps, ["thrust"]); } + +/// The evaluated view of a one-file project. +fn evaluated_view_of_source(source: &str) -> ViewModel { + let mut fs = graphcal_io::InMemoryFileSystem::new(); + fs.add_file( + graphcal_io::VirtualAbsolutePath::new("/proj/view.gcl").unwrap(), + source.to_string(), + ) + .unwrap(); + let project = load_project(Path::new("/proj/view.gcl"), Some(Path::new("/proj")), &fs).unwrap(); + let host_fns = graphcal_eval::host_fns::demo_registry(); + let checked = ProjectCompiler::new(&project) + .host_fns(&host_fns) + .check() + .unwrap(); + let mut projection = project_view(&project, &checked, None).unwrap(); + let prepared = checked.prepare_with_host_fns(&host_fns).unwrap(); + let evaluated = prepared + .evaluate(&prepared.binding_builder().finish().unwrap()) + .unwrap(); + projection.attach_evaluation(&evaluated).unwrap(); + projection.into_model() +} + +fn display_of(view: &ViewModel, id: &str) -> Option { + match &node(view, id).value { + Some(ViewValue::Quantity { display, .. }) => Some(display.clone()), + _ => None, + } +} + +#[test] +fn inline_dag_templates_stay_out_of_the_sheet() { + // Both templates declare `x` and `out`; only their instances are read. + let view = evaluated_view_of_source( + "\ +dag one { + param x: Dimensionless = 1.0; + pub node out: Dimensionless = @x; +} +dag two { + param x: Dimensionless = 2.0; + pub node out: Dimensionless = @x; +} +include one() as a; +include two() as b; +node y: Dimensionless = @a::out + @b::out; +", + ); + let mut ids: Vec<&str> = view.nodes.iter().map(|node| node.id.as_str()).collect(); + ids.sort_unstable(); + assert_eq!(ids, ["a::out", "b::out", "y"]); + assert_eq!(node(&view, "y").deps, ["a::out", "b::out"]); + assert_eq!(node(&view, "a::out").kind, ViewNodeKind::External); + assert_eq!(display_of(&view, "a::out").as_deref(), Some("1")); + assert_eq!(display_of(&view, "b::out").as_deref(), Some("2")); + assert_eq!(display_of(&view, "y").as_deref(), Some("3")); +} + +#[test] +fn a_root_declaration_named_like_a_template_member_keeps_its_own_node() { + let view = evaluated_view_of_source( + "\ +dag checked { + param v: Dimensionless; + pub node out: Dimensionless = @v * 2.0; +} +node out: Dimensionless = @checked(v: 3.0)::out; +include checked(v: 4.0) as also_fine; +node z: Dimensionless = @out + @also_fine::out; +", + ); + let ids: Vec<&str> = view.nodes.iter().map(|node| node.id.as_str()).collect(); + assert_eq!(ids, ["out", "z", "also_fine::out"]); + assert_eq!(node(&view, "z").deps, ["out", "also_fine::out"]); + assert_eq!(display_of(&view, "out").as_deref(), Some("6")); + assert_eq!(display_of(&view, "also_fine::out").as_deref(), Some("8")); +} + +fn bare_node(id: &str, deps: &[&str]) -> ViewNode { + ViewNode { + id: id.to_string(), + name: id.to_string(), + module: "m".to_string(), + kind: ViewNodeKind::Node, + axes: Vec::new(), + dimension: None, + unit: None, + is_public: false, + deps: deps.iter().map(ToString::to_string).collect(), + expr: None, + reduce: None, + instance: None, + doc: None, + source: None, + value: None, + } +} + +#[test] +fn node_references_reject_a_shared_id_and_a_dangling_dependency() { + use super::project::check_node_references; + + assert!(check_node_references(&[bare_node("a", &[]), bare_node("b", &["a"])]).is_ok()); + assert!(matches!( + check_node_references(&[bare_node("a", &[]), bare_node("a", &[])]), + Err(ViewProjectionError::DuplicateNodeId { id }) if id == "a" + )); + assert!(matches!( + check_node_references(&[bare_node("b", &["a"])]), + Err(ViewProjectionError::UnknownDependency { node, dependency }) + if node == "b" && dependency == "a" + )); +} From 6c1e0edcd1d11f873fd42988629fd738f68b1239 Mon Sep 17 00:00:00 2001 From: Shunichiro Nomura Date: Wed, 14 Oct 2026 20:51:10 +0900 Subject: [PATCH 4/8] web/view: a patch says which last line lacks a newline MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The diff dropped each line's newline and put one "\ No newline at end of file" after the last hunk, whichever line it belonged to. git applies that marker to the line before it, so an edit far from a file's newline-less end joined two lines (`param p4: …;param p5: …;`), and a changed last line, or a change of the final newline alone, gave a patch git rejects. Lines now keep their newline when compared, so `b` and `b\n` differ, and the marker follows each newline-less line on its own side; an empty side's hunk starts at line 0. Tests round-trip patches through git apply. Co-Authored-By: Claude Opus 5.5 --- web/view/src/diff.test.ts | 70 +++++++++++++++++++++++++++++++++++++-- web/view/src/diff.ts | 25 ++++++++------ 2 files changed, 81 insertions(+), 14 deletions(-) diff --git a/web/view/src/diff.test.ts b/web/view/src/diff.test.ts index ead912c0a..c8eee234e 100644 --- a/web/view/src/diff.test.ts +++ b/web/view/src/diff.test.ts @@ -1,3 +1,7 @@ +import { execFileSync } from "node:child_process"; +import { mkdtempSync, readFileSync, rmSync, writeFileSync } from "node:fs"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; import { describe, expect, test } from "vite-plus/test"; import { patchOf, unifiedDiff } from "./diff.ts"; @@ -39,7 +43,8 @@ describe("unifiedDiff", () => { expect(heads).toEqual(["@@ -1,9 +1,9 @@", "@@ -23,7 +23,7 @@"]); }); - test("handles added and removed lines and a missing final newline", () => { + test("handles added and removed lines and a dropped final newline", () => { + // `b` loses its newline, so it is changed, and the marker follows the new `b`. const diff = unifiedDiff({ path: "f", before: "a\nb\n", after: "a\nx\ny\nb" }); expect(diff).toBe( [ @@ -48,9 +53,10 @@ describe("unifiedDiff", () => { "+++ b/f", "@@ -1,2 +1,4 @@", " a", + "-b", "+x", "+y", - " b", + "+b", "\\ No newline at end of file", "", ].join("\n"), @@ -58,7 +64,65 @@ describe("unifiedDiff", () => { expect(unifiedDiff({ path: "f", before: "a\n", after: "a\n" })).toBe(""); }); - test("patchOf puts a heading above the changed files only", () => { + test("marks the old and the new last line separately", () => { + expect(unifiedDiff({ path: "f", before: "a", after: "b" })).toBe( + [ + "diff --git a/f b/f", + "--- a/f", + "+++ b/f", + "@@ -1,1 +1,1 @@", + "-a", + "\\ No newline at end of file", + "+b", + "\\ No newline at end of file", + "", + ].join("\n"), + ); + }); + + test("a change far from a last line without a newline leaves it unmarked", () => { + const lines = Array.from({ length: 20 }, (_, i) => `param p${i}: Length = ${i}.0 m;`); + const before = lines.join("\n"); + const diff = unifiedDiff({ path: "f", before, after: before.replace("1.0 m", "9.0 m") }); + expect(diff).not.toContain("No newline"); + }); +}); + +/** What `git apply` makes of `before` with the patch: the bytes a user ends up with. */ +function applied(before: string, after: string): string { + const dir = mkdtempSync(join(tmpdir(), "graphcal-view-diff-")); + try { + writeFileSync(join(dir, "f.gcl"), before); + writeFileSync(join(dir, "p.patch"), unifiedDiff({ path: "f.gcl", before, after })); + execFileSync("git", ["apply", "p.patch"], { cwd: dir, stdio: "pipe" }); + return readFileSync(join(dir, "f.gcl"), "utf8"); + } finally { + rmSync(dir, { recursive: true, force: true }); + } +} + +describe("git apply round trip", () => { + const lines = Array.from({ length: 20 }, (_, i) => `param p${i}: Length = ${i}.0 m;`); + const text = lines.join("\n"); + test.each([ + ["an edit far from a last line without a newline", text, text.replace("1.0 m", "9.0 m")], + ["an edit that also drops the final newline", `${text}\n`, text.replace("1.0 m", "9.0 m")], + ["a changed last line on both sides without one", text, text.replace("19.0 m", "9.0 m")], + ["only the final newline added", "a\nb", "a\nb\n"], + ["only the final newline dropped", "a\nb\n", "a\nb"], + ["a line without a newline replaced by one with", "a", "b\n"], + ["a line with a newline replaced by one without", "a\n", "b"], + ["an empty file filled", "", "a\nb\n"], + ["a file emptied", "a\nb\n", ""], + ["CRLF lines", "a\r\nb\r\nc\r\n", "a\r\nB\r\nc\r\n"], + ["two hunks, the last at the end", `${text}\n`, `${text.replace("1.0 m", "9.0 m")}\nx`], + ])("%s", (_name, before, after) => { + expect(applied(before, after)).toBe(after); + }); +}); + +describe("patchOf", () => { + test("puts a heading above the changed files only", () => { const patch = patchOf( [ { path: "a", before: "1\n", after: "1\n" }, diff --git a/web/view/src/diff.ts b/web/view/src/diff.ts index 219a9a806..8ecf7e23d 100644 --- a/web/view/src/diff.ts +++ b/web/view/src/diff.ts @@ -14,12 +14,12 @@ interface Op { text: string; } -/** Lines of a text, the final newline not making an extra empty line. */ +/** + * Lines of a text, each with its terminating newline. Only a last line can lack + * one, so `b` and `b\n` differ: a patch must carry an added or dropped final newline. + */ function linesOf(text: string): string[] { - if (text === "") return []; - const lines = text.split("\n"); - if (lines.at(-1) === "") lines.pop(); - return lines; + return text.match(/[^\n]*\n|[^\n]+$/g) ?? []; } /** The edit script between two line arrays: common prefix and suffix kept, the middle by LCS. */ @@ -120,18 +120,21 @@ export function unifiedDiff(change: FileChange): string { const slice = ops.slice(hunk.from, hunk.to); const oldCount = slice.filter((op) => op.kind !== "+").length; const newCount = slice.filter((op) => op.kind !== "-").length; - out.push(`@@ -${oldLine},${oldCount} +${newLine},${newCount} @@`); + // An empty side starts before its first line, as `@@ -0,0 +1 @@` does. + const startOf = (line: number, count: number): number => (count === 0 ? line - 1 : line); + out.push( + `@@ -${startOf(oldLine, oldCount)},${oldCount} +${startOf(newLine, newCount)},${newCount} @@`, + ); for (const op of slice) { - out.push(`${op.kind}${op.text}`); + const ended = op.text.endsWith("\n"); + out.push(`${op.kind}${ended ? op.text.slice(0, -1) : op.text}`); + // The marker follows the line it is about: a file's last line, without a newline. + if (!ended) out.push("\\ No newline at end of file"); if (op.kind !== "+") oldLine += 1; if (op.kind !== "-") newLine += 1; } at = hunk.to; } - const missingNewline = (text: string): boolean => text !== "" && !text.endsWith("\n"); - if (missingNewline(change.before) || missingNewline(change.after)) { - out.push("\\ No newline at end of file"); - } return `${out.join("\n")}\n`; } From e1ff01b7a4c634d33374a8561a1097477ea0815d Mon Sep 17 00:00:00 2001 From: Shunichiro Nomura Date: Wed, 14 Oct 2026 20:52:11 +0900 Subject: [PATCH 5/8] web/view: a table cell edit stays inside its declaration A cell edit took the declaration's table to run from its line to the first later line holding only `}`. After a table closed on one line, that was the next declaration's table: editing `a[x]` rewrote `b`'s `x`, which compiled, so the edit was accepted, entered the copied patch, and in VS Code reached the document. The edit now searches only the rows strictly inside the compiler's range of the expression, which must begin with `table[`; a table on one line, or one shared by several declarations (its axes name a slot tuple), is refused rather than guessed at. A stopgap: the compiler is to report each cell's range. Co-Authored-By: Claude Opus 5.5 --- web/view/src/main.ts | 5 ++- web/view/src/source.test.ts | 83 +++++++++++++++++++++++++++++++++++++ web/view/src/source.ts | 42 ++++++++++++------- 3 files changed, 112 insertions(+), 18 deletions(-) create mode 100644 web/view/src/source.test.ts diff --git a/web/view/src/main.ts b/web/view/src/main.ts index 4ddbec2a4..fc034a272 100644 --- a/web/view/src/main.ts +++ b/web/view/src/main.ts @@ -917,14 +917,15 @@ function editCell( ): Promise { if (row === null) return editExpr(s, node, literalFor(node, text)); const at = sourceOf(s, node); - if (at === null || node.source === null) { + const range = node.source?.expr_range ?? null; + if (at === null || range === null) { showNote("this value cannot be edited"); return Promise.resolve(false); } const { file, source } = at; let rewritten: string; try { - rewritten = replaceTableCell(source, node.source.line, row, sub, literalFor(node, text)); + rewritten = replaceTableCell(source, range, row, sub, literalFor(node, text)); } catch (error) { showNote(error instanceof EditError ? error.message : errorMessage(error)); return Promise.resolve(false); diff --git a/web/view/src/source.test.ts b/web/view/src/source.test.ts new file mode 100644 index 000000000..e1a1fbdf8 --- /dev/null +++ b/web/view/src/source.test.ts @@ -0,0 +1,83 @@ +import { describe, expect, test } from "vite-plus/test"; +import { EditError, type Range, replaceTableCell } from "./source.ts"; + +/** The range of the expression that starts at `table[` on 0-based `line` and ends after the `}` on `endLine`. */ +function tableRange(source: string, line: number, endLine: number): Range { + const rows = source.split("\n"); + const start = (rows[line] ?? "").indexOf("table["); + const end = (rows[endLine] ?? "").lastIndexOf("}") + 1; + return { start: { line, character: start }, end: { line: endLine, character: end } }; +} + +describe("replaceTableCell", () => { + test("replaces a one-axis entry of its own table", () => { + const source = ["param p: Power[C] = table[C] {", " a: 1.0 W;", " b: 2.0 W;", "};"].join( + "\n", + ); + expect(replaceTableCell(source, tableRange(source, 0, 3), "b", null, "5.0 W")).toBe( + source.replace("2.0 W", "5.0 W"), + ); + }); + + test("replaces the column a header names", () => { + const source = [ + "param d: Dimensionless[C, M] = table[C, M] {", + " : Safe, Nominal;", + " a: 0.1, 0.2;", + "};", + ].join("\n"); + expect(replaceTableCell(source, tableRange(source, 0, 3), "a", "Nominal", "0.5")).toBe( + source.replace("0.2", "0.5"), + ); + }); + + test("never reaches the next declaration's table", () => { + // `a` closes on its own line; a search past its range would find `b`'s row `x`. + const source = [ + "param a: Power[M] = table[M] { x: 1.0 W; y: 2.0 W; };", + "param b: Power[M] = table[M] {", + " x: 3.0 W;", + " y: 4.0 W;", + "};", + ].join("\n"); + expect(() => replaceTableCell(source, tableRange(source, 0, 0), "x", null, "9.0 W")).toThrow( + EditError, + ); + const edited = replaceTableCell(source, tableRange(source, 1, 4), "x", null, "9.0 W"); + expect(edited).toBe(source.replace("3.0 W", "9.0 W")); + }); + + test("a row outside the range is not found", () => { + const source = [ + "param a: Power[M] = table[M] {", + " y: 2.0 W;", + "};", + "param b: Power[M] = table[M] {", + " x: 3.0 W;", + "};", + ].join("\n"); + expect(() => replaceTableCell(source, tableRange(source, 0, 2), "x", null, "9.0 W")).toThrow( + "row x is not in the table literal", + ); + }); + + test("refuses an expression that is not a table literal", () => { + const source = "node n: Power[M] = for m: M {\n 1.0 W\n};"; + const range = { start: { line: 0, character: 19 }, end: { line: 2, character: 1 } }; + expect(() => replaceTableCell(source, range, "x", null, "9.0 W")).toThrow(EditError); + }); + + test("refuses a table shared by several declarations", () => { + const source = [ + "param p: Power[C],", + "param n: Int[C]", + " = table[C, (_, _)] {", + " : _, _;", + " a: 10.0 W, 1;", + " };", + ].join("\n"); + expect(() => replaceTableCell(source, tableRange(source, 2, 5), "a", null, "9.0 W")).toThrow( + "a table shared by several declarations cannot be edited cell by cell", + ); + }); +}); diff --git a/web/view/src/source.ts b/web/view/src/source.ts index 576e49e68..2f51b73a8 100644 --- a/web/view/src/source.ts +++ b/web/view/src/source.ts @@ -27,6 +27,12 @@ export interface Position { character: number; } +/** A half-open editor range: 0-based lines and UTF-16 columns. */ +export interface Range { + start: Position; + end: Position; +} + function offsetOf(source: string, position: Position): number { const rows = lines(source); if (position.line >= rows.length) return source.length; @@ -35,27 +41,30 @@ function offsetOf(source: string, position: Position): number { } /** Replace the text between two positions, as the compiler reports an expression's range. */ -export function replaceRange( - source: string, - range: { start: Position; end: Position }, - to: string, -): string { +export function replaceRange(source: string, range: Range, to: string): string { const from = offsetOf(source, range.start); const until = offsetOf(source, range.end); if (until < from) throw new EditError("the expression range is inverted"); return source.slice(0, from) + to + source.slice(until); } -/** The `[start, end]` 0-based row range of the `table … { … };` starting at `line`. */ -function tableBlock(rows: string[], line: number): [number, number] { - const start = line - 1; - if (!/\btable\s*\[/.test(rows[start] ?? "")) { - throw new EditError("this value is not a table literal in the source"); +/** + * The 0-based rows strictly inside the table literal that `range` spans: the + * rows between its `table[…] {` line and its closing line. Only these rows are + * searched, so an edit never reaches past the declaration's own expression. + */ +function tableRows(rows: string[], range: Range): [number, number] { + const head = (rows[range.start.line] ?? "").slice(range.start.character); + const axes = /^\s*table\s*\[([^\]]*)\]/.exec(head); + if (!axes) throw new EditError("this value is not a table literal in the source"); + // A multi-declaration table's columns belong to several declarations. + if ((axes[1] ?? "").includes("(")) { + throw new EditError("a table shared by several declarations cannot be edited cell by cell"); } - for (let i = start; i < rows.length; i += 1) { - if (/^\s*}\s*;?\s*$/.test(rows[i] ?? "")) return [start, i]; + if (range.end.line <= range.start.line) { + throw new EditError("a table literal on one line cannot be edited cell by cell"); } - throw new EditError("the table literal has no closing brace"); + return [range.start.line + 1, range.end.line]; } /** Split a table row body `a, b, c` keeping each item's surrounding spaces. */ @@ -66,19 +75,20 @@ function splitItems(body: string): string[] { /** * Replace one entry of a table literal: `row: value;` for one axis, or the * `sub` column of `row: v1, v2, …;` under a `: sub1, sub2, …;` header. + * `range` is the compiler's range of the declaration's expression. */ export function replaceTableCell( source: string, - line: number, + range: Range, row: string, sub: string | null, to: string, ): string { const rows = lines(source); - const [start, end] = tableBlock(rows, line); + const [start, end] = tableRows(rows, range); const rowRe = new RegExp(`^(\\s*${row}\\s*:\\s*)(.*?)(\\s*;.*)$`); let header: string[] | null = null; - for (let i = start + 1; i < end; i += 1) { + for (let i = start; i < end; i += 1) { const text = rows[i] ?? ""; const head = /^\s*:\s*(.*?)\s*;/.exec(text); if (head) { From 6577ad1fabd58c26858fa4aed0d04b6a408eff67 Mon Sep 17 00:00:00 2001 From: Shunichiro Nomura Date: Wed, 14 Oct 2026 20:53:00 +0900 Subject: [PATCH 6/8] web/view: a nonzero value never shows as 0 in a cell MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A cell rounded the magnitude to four significant digits and then printed at most six decimals, so anything under 1e-6 became 0 (4.7e-9 → 0, -3e-8 → -0) and a value near 1e-6 lost its digits; large values printed in full (1e308 as 309 digits). Below 1e-3 and from 1e6 the four digits take an exponent instead. Editing still starts from the full magnitude. Co-Authored-By: Claude Opus 5.5 --- web/view/src/values.test.ts | 35 +++++++++++++++++++++++++++++++++++ web/view/src/values.ts | 9 ++++++++- 2 files changed, 43 insertions(+), 1 deletion(-) create mode 100644 web/view/src/values.test.ts diff --git a/web/view/src/values.test.ts b/web/view/src/values.test.ts new file mode 100644 index 000000000..e9c3ebc82 --- /dev/null +++ b/web/view/src/values.test.ts @@ -0,0 +1,35 @@ +import { describe, expect, test } from "vite-plus/test"; +import { shortMagnitude } from "./values.ts"; + +describe("shortMagnitude", () => { + test.each([ + ["0", "0"], + ["-0", "0"], + ["1", "1"], + ["123456", "123500"], + ["999999", "1e+6"], + ["123456789", "1.235e+8"], + ["-2.5", "-2.5"], + ["0.001", "0.001"], + ["0.0012346", "0.001235"], + ["0.123456", "0.1235"], + ["0.000999", "9.99e-4"], + ["0.000001", "1e-6"], + ["1.2346e-6", "1.235e-6"], + ["4.7e-9", "4.7e-9"], + ["-3e-8", "-3e-8"], + ["1e308", "1e+308"], + ["not a number", "not a number"], + ])("%s → %s", (magnitude, shown) => { + expect(shortMagnitude(magnitude)).toBe(shown); + }); + + test("a nonzero value never reads as zero", () => { + for (let exponent = -320; exponent <= 0; exponent += 1) { + for (const sign of [1, -1]) { + const shown = shortMagnitude(String(sign * 1.5 * 10 ** exponent)); + expect(Number(shown), shown).not.toBe(0); + } + } + }); +}); diff --git a/web/view/src/values.ts b/web/view/src/values.ts index 6be130e30..e013ffdfa 100644 --- a/web/view/src/values.ts +++ b/web/view/src/values.ts @@ -15,11 +15,18 @@ export function displayOf(value: Value | null): string { return value === null ? "—" : value.display; } -/** A magnitude rounded for a cell: four significant digits, no grouping. */ +/** + * A magnitude rounded for a cell: four significant digits, no grouping. Below + * 1e-3 those digits need more than six decimals, and from 1e6 the zeros only + * pad them, so the value takes an exponent instead: a nonzero value never + * reads as 0, and a large one stays short. + */ export function shortMagnitude(magnitude: string): string { const x = Number(magnitude); if (!Number.isFinite(x)) return magnitude; + if (x === 0) return "0"; const rounded = Number(x.toPrecision(4)); + if (Math.abs(rounded) < 1e-3 || Math.abs(rounded) >= 1e6) return rounded.toExponential(); return rounded.toLocaleString("en-US", { maximumFractionDigits: 6, useGrouping: false }); } From f6fc74db7ef6c5055c1a223dd8e8ba1887241159 Mon Sep 17 00:00:00 2001 From: Shunichiro Nomura Date: Wed, 14 Oct 2026 20:58:06 +0900 Subject: [PATCH 7/8] view: no pane claims to show a bound include's values A pinned include pane showed its module evaluated alone, though the include passes arguments: `include lib(gain: 2.0) as p` read `p::out = 2` while the pane "lib.gcl as p" showed `out = 1`, wired to that 2 as its producer. The view model's `instance` is now `{ name, bound }`, bound when the include binds a param or a Static parameter. A bound include cannot be pinned yet (its menu entry says so), and no cross wire joins a bound include's value to a pane. A stopgap until the view can project an instance with its arguments. Co-Authored-By: Claude Opus 5.5 --- crates/graphcal-project/src/view_model/mod.rs | 4 +- .../graphcal-project/src/view_model/model.rs | 21 ++++-- .../src/view_model/project.rs | 43 +++++++----- .../graphcal-project/src/view_model/tests.rs | 29 ++++++++- web/view/src/cross.test.ts | 65 +++++++++++++++++++ web/view/src/cross.ts | 7 +- web/view/src/main.ts | 16 +++-- web/view/src/model.ts | 8 ++- web/view/src/panes.ts | 9 ++- 9 files changed, 164 insertions(+), 38 deletions(-) create mode 100644 web/view/src/cross.test.ts diff --git a/crates/graphcal-project/src/view_model/mod.rs b/crates/graphcal-project/src/view_model/mod.rs index 5a3882610..52f57c20d 100644 --- a/crates/graphcal-project/src/view_model/mod.rs +++ b/crates/graphcal-project/src/view_model/mod.rs @@ -19,7 +19,7 @@ mod tests; mod value; pub use model::{ - ViewEntry, ViewIndex, ViewModel, ViewModule, ViewNode, ViewNodeKind, ViewPosition, ViewRange, - ViewReduce, ViewSource, ViewValue, + ViewEntry, ViewIndex, ViewInstance, ViewModel, ViewModule, ViewNode, ViewNodeKind, + ViewPosition, ViewRange, ViewReduce, ViewSource, ViewValue, }; pub use project::{ViewProjection, ViewProjectionError, project_view}; diff --git a/crates/graphcal-project/src/view_model/model.rs b/crates/graphcal-project/src/view_model/model.rs index 425feb504..88b71f2ab 100644 --- a/crates/graphcal-project/src/view_model/model.rs +++ b/crates/graphcal-project/src/view_model/model.rs @@ -80,11 +80,10 @@ pub struct ViewNode { pub expr: Option, /// The aggregation that removed an axis of a dependency. Not derived yet. pub reduce: Option, - /// For an external read through an `include`: the instance's name - /// (`eps` in `include power_project.eps() …`, or the alias of - /// `include … as e`). `None` for the module's own declarations and for - /// imports, which have no instance. - pub instance: Option, + /// For an external read through an `include`: the instance it is read + /// through. `None` for the module's own declarations and for imports, + /// which have no instance. + pub instance: Option, /// The declaration's `///` documentation: one line per `///` line, /// joined with newlines. The view shows its first line beside the name. pub doc: Option, @@ -94,6 +93,18 @@ pub struct ViewNode { pub value: Option, } +/// An include instance a node is read through. +#[derive(Debug, Clone, PartialEq, Eq, Serialize)] +pub struct ViewInstance { + /// The instance's name: `eps` in `include power_project.eps() …`, or + /// the alias of `include … as e`. + pub name: String, + /// Whether the include binds any of the template's params or Static + /// parameters. A bound instance's values differ from those of its + /// module evaluated alone, which is all a view of that module shows. + pub bound: bool, +} + /// An aggregation over one axis. #[derive(Debug, Clone, Serialize)] pub struct ViewReduce { diff --git a/crates/graphcal-project/src/view_model/project.rs b/crates/graphcal-project/src/view_model/project.rs index a6150ef16..ae5c81821 100644 --- a/crates/graphcal-project/src/view_model/project.rs +++ b/crates/graphcal-project/src/view_model/project.rs @@ -31,7 +31,7 @@ use crate::loader::{LoadedFile, LoadedProject}; use crate::project_compiler::CheckedProject; use super::model::{ - ViewIndex, ViewModel, ViewModule, ViewNode, ViewNodeKind, ViewSource, ViewValue, + ViewIndex, ViewInstance, ViewModel, ViewModule, ViewNode, ViewNodeKind, ViewSource, ViewValue, }; use super::value::{CoherentUnits, common_unit, view_value}; @@ -368,7 +368,7 @@ impl<'p> Scope<'p> { IncludeAlias { target: projection.target, template: template.clone(), - instance: instance_label(record, template), + instance: view_instance(record, template), }, ); } @@ -403,7 +403,7 @@ impl<'p> Scope<'p> { .instance .exposed_name(projection.projection) .to_string(), - instance: Some(instance_label(record, template)), + instance: Some(view_instance(record, template)), }, ); } @@ -412,19 +412,26 @@ impl<'p> Scope<'p> { } } -/// The name the view shows for an include instance: its alias -/// (`include lib() as l`), or the template's file name when the include -/// is anonymous. -fn instance_label( +/// An include instance as the view shows it: named by its alias +/// (`include lib() as l`), or the template's file name when the include is +/// anonymous, and whether the include binds anything. +fn view_instance( record: &graphcal_compiler::ir::instance::HirInstanceRecord, template: &DagId, -) -> String { - record - .instance - .id() - .scope() - .alias() - .map_or_else(|| template.leaf().to_string(), ToString::to_string) +) -> ViewInstance { + let substitution = record.instance.substitution(); + ViewInstance { + name: record + .instance + .id() + .scope() + .alias() + .map_or_else(|| template.leaf().to_string(), ToString::to_string), + bound: !record.value_bindings.is_empty() + || !substitution.indexes.is_empty() + || !substitution.types.is_empty() + || !substitution.dimensions.is_empty(), + } } /// A root declaration standing for an include instance's output. @@ -433,15 +440,15 @@ struct IncludeAlias { target: ResolvedDeclName, /// The template the instance was materialized from. template: DagId, - /// The instance's name in the view. - instance: String, + /// The instance as the view shows it. + instance: ViewInstance, } /// How the root spells a declaration of another module. struct ExternalName { id: String, /// The include instance it is read through, if any. - instance: Option, + instance: Option, } /// What each local declaration reads: dataflow edges for values, the @@ -692,7 +699,7 @@ struct NodeInput<'a> { module: String, /// Overrides the kind derived from the declaration (for externals). kind: Option, - instance: Option, + instance: Option, is_public: bool, deps: Vec, file: Option>, diff --git a/crates/graphcal-project/src/view_model/tests.rs b/crates/graphcal-project/src/view_model/tests.rs index 710a09886..81ed39ef4 100644 --- a/crates/graphcal-project/src/view_model/tests.rs +++ b/crates/graphcal-project/src/view_model/tests.rs @@ -125,7 +125,9 @@ fn power_max_structure_and_values() { assert_eq!(budget.kind, ViewNodeKind::External); assert_eq!(budget.module, "power_project.eps"); assert_eq!(budget.deps, Vec::::new()); - assert_eq!(budget.instance.as_deref(), Some("eps")); + // `include power_project.eps()` binds nothing. + let instance = budget.instance.as_ref().map(|i| (i.name.as_str(), i.bound)); + assert_eq!(instance, Some(("eps", false))); assert_eq!(budget.source.as_ref().unwrap().line, 23); assert!( budget @@ -297,3 +299,28 @@ fn node_references_reject_a_shared_id_and_a_dangling_dependency() { if node == "b" && dependency == "a" )); } + +#[test] +fn an_include_with_arguments_is_marked_bound() { + let view = evaluated_view_of_source( + "\ +dag lib { + param gain: Dimensionless = 1.0; + pub node out: Dimensionless = @gain; +} +include lib(gain: 2.0) as p; +include lib() as q; +node y: Dimensionless = @p::out + @q::out; +", + ); + let instance = |id: &str| node(&view, id).instance.clone().expect("an include member"); + assert_eq!(instance("p::out").name, "p"); + assert!(instance("p::out").bound); + assert_eq!(instance("q::out").name, "q"); + assert!(!instance("q::out").bound); + assert_eq!( + node(&view, "y").instance.as_ref().map(|i| i.name.as_str()), + None + ); + assert_eq!(display_of(&view, "p::out").as_deref(), Some("2")); +} diff --git a/web/view/src/cross.test.ts b/web/view/src/cross.test.ts new file mode 100644 index 000000000..69f51e3ea --- /dev/null +++ b/web/view/src/cross.test.ts @@ -0,0 +1,65 @@ +import { describe, expect, test } from "vite-plus/test"; +import { crossLinks, type CrossSheet } from "./cross.ts"; +import { computeLayout } from "./layout.ts"; +import type { ViewModel, ViewNode } from "./model.ts"; +import { keyOf, type PaneKey } from "./panes.ts"; + +function node(id: string, extra: Partial = {}): ViewNode { + return { + id, + name: id, + module: "p.main", + kind: "node", + axes: [], + dimension: "Dimensionless", + unit: null, + pub: false, + deps: [], + expr: null, + reduce: null, + instance: null, + source: null, + value: null, + ...extra, + }; +} + +function sheet(key: PaneKey, id: string, nodes: ViewNode[]): CrossSheet { + const model: ViewModel = { + version: 0, + module: { id, file: `src/${id}.gcl`, package: "p" }, + indexes: [], + nodes, + }; + // The links read only the models and layouts; no element is touched. + const element = {} as HTMLElement; + return { key, model, layout: computeLayout(model), viewEl: element, paneEl: element }; +} + +/** The main sheet reading `lib`'s `out` through the include `l`, and `lib` pinned as that include. */ +function sheets(bound: boolean): CrossSheet[] { + const external = node("l::out", { + name: "out", + module: "p.lib", + kind: "external", + instance: { name: "l", bound }, + }); + const main = sheet({ module: "p.main", instance: null }, "p.main", [ + external, + node("y", { deps: ["l::out"] }), + ]); + const lib = sheet(keyOf(external), "p.lib", [node("out", { module: "p.lib", pub: true })]); + return [main, lib]; +} + +describe("crossLinks", () => { + test("wires an unbound include's value to its pane", () => { + expect(crossLinks(sheets(false)).map((link) => [link.fromId, link.toId])).toEqual([ + ["out", "l::out"], + ]); + }); + + test("never wires a bound include's value to the module evaluated alone", () => { + expect(crossLinks(sheets(true))).toEqual([]); + }); +}); diff --git a/web/view/src/cross.ts b/web/view/src/cross.ts index 718f85e48..f00acf86d 100644 --- a/web/view/src/cross.ts +++ b/web/view/src/cross.ts @@ -4,7 +4,7 @@ // box. import type { Layout } from "./layout.ts"; import type { ViewModel } from "./model.ts"; -import { sameKey, type PaneKey } from "./panes.ts"; +import { keyOf, sameKey, type PaneKey } from "./panes.ts"; import { anchorsOf, CHANNEL_PAD, @@ -43,8 +43,9 @@ export function crossLinks(sheets: CrossSheet[]): CrossLink[] { for (const to of sheets) { const tiers = scalarTierCount(to.layout); for (const node of to.model.nodes) { - if (node.kind !== "external") continue; - const key = { module: node.module, instance: node.instance }; + // A pane shows its module evaluated alone, which a bound include's values are not. + if (node.kind !== "external" || node.instance?.bound === true) continue; + const key = keyOf(node); const from = sheets.find((s) => sameKey(s.key, key)); const column = to.layout.columns.get(node.id); if (!from || !column || !from.layout.columns.has(node.name)) continue; diff --git a/web/view/src/main.ts b/web/view/src/main.ts index fc034a272..6d6e32405 100644 --- a/web/view/src/main.ts +++ b/web/view/src/main.ts @@ -26,7 +26,7 @@ import { GitHubHost } from "./githubHost.ts"; import type { Host, NoteKind } from "./host.ts"; import { TokenStore } from "./login.ts"; import type { ViewModel, ViewNode } from "./model.ts"; -import { depthsOf, sameKey, type PaneKey } from "./panes.ts"; +import { depthsOf, keyOf, sameKey, type PaneKey } from "./panes.ts"; import { parseRoute } from "./route.ts"; import { EditError, replaceExpr, replaceRange, replaceTableCell } from "./source.ts"; import { EmptyHost, mountStart } from "./start.ts"; @@ -1166,8 +1166,9 @@ function rowMenu(s: Sheet, label: HTMLElement, event: MouseEvent): void { /** `Open eps.gcl` goes there (the trail leads back); `Pin eps beside` opens it as a pane. */ function externalMenu(w: Workspace, node: ViewNode, event: MouseEvent): void { - const key = { module: node.module, instance: node.instance }; + const key = keyOf(node); const file = fileNameOfModule(node.module); + const name = node.instance?.name ?? file; const pinned = w.sheets.find((other) => sameKey(other.key, key)); openMenu(event.clientX, event.clientY, [ { @@ -1178,8 +1179,15 @@ function externalMenu(w: Workspace, node: ViewNode, event: MouseEvent): void { }, }, pinned - ? { label: `Unpin ${node.instance ?? file}`, run: () => unpin(pinned) } - : { label: `Pin ${node.instance ?? file} beside`, run: () => pin(key) }, + ? { label: `Unpin ${name}`, run: () => unpin(pinned) } + : node.instance?.bound === true + ? // A pane would show the module evaluated alone, not this include's arguments. + { + label: `Pin ${name} beside: not yet for an include with arguments`, + disabled: true, + run: () => undefined, + } + : { label: `Pin ${name} beside`, run: () => pin(key) }, ]); } diff --git a/web/view/src/model.ts b/web/view/src/model.ts index 489397821..4907ffde7 100644 --- a/web/view/src/model.ts +++ b/web/view/src/model.ts @@ -65,10 +65,12 @@ export const viewNodeSchema = z.object({ /** The aggregation that removed an axis of a dependency, if any. */ reduce: z.object({ op: reduceOpSchema, axis: z.string() }).nullable(), /** - * For an external: the include instance it is read from (`eps` in - * `include power_project.eps() …`); null for an import, which has no instance. + * For an external: the include instance it is read from, named as `eps` in + * `include power_project.eps() …`; null for an import, which has no instance. + * A bound instance (the include passes arguments) has values of its own, + * which no view of its module evaluated alone shows. */ - instance: z.string().nullable(), + instance: z.object({ name: z.string(), bound: z.boolean() }).nullable(), /** The declaration's `///` documentation, lines joined with newlines. */ doc: z.string().nullable().optional(), source: z diff --git a/web/view/src/panes.ts b/web/view/src/panes.ts index dff30ebf7..d2e072750 100644 --- a/web/view/src/panes.ts +++ b/web/view/src/panes.ts @@ -3,7 +3,7 @@ // or an imported module. The main sheet is the rightmost column; a pane sits // one column left of its deepest open consumer, and a module read from two // places opens once. -import type { ViewModel } from "./model.ts"; +import type { ViewModel, ViewNode } from "./model.ts"; export interface PaneKey { module: string; @@ -15,6 +15,11 @@ export function sameKey(a: PaneKey, b: PaneKey): boolean { return a.module === b.module && a.instance === b.instance; } +/** The pane an external node is read from. */ +export function keyOf(node: ViewNode): PaneKey { + return { module: node.module, instance: node.instance?.name ?? null }; +} + /** The panes a module reads from: one per include instance, one per imported module. */ export function references(model: ViewModel): PaneKey[] { const out: PaneKey[] = []; @@ -22,7 +27,7 @@ export function references(model: ViewModel): PaneKey[] { if (!out.some((k) => sameKey(k, key))) out.push(key); }; for (const node of model.nodes) { - if (node.kind === "external") add({ module: node.module, instance: node.instance }); + if (node.kind === "external") add(keyOf(node)); } for (const index of model.indexes) { if (index.module !== model.module.id) add({ module: index.module, instance: null }); From 346197b46723e8b38a98e0ce74f44159f06a7ff1 Mon Sep 17 00:00:00 2001 From: Shunichiro Nomura Date: Wed, 14 Oct 2026 21:00:26 +0900 Subject: [PATCH 8/8] web/view: a sheet opened after an edit shows the edited sources An edit re-projected only the modules on screen; every other module kept its model from before the edit, and opening or pinning it later showed those values (b = 1 after a, which b reads, became 3), with declarations, axes and source ranges as old as the values. The host's models now live in a ModelCache that marks every model the edit did not re-project as stale and never returns a stale one as current. Asking for, opening or pinning a stale module projects it again from the current sources first; a reset makes the models as read current again. Co-Authored-By: Claude Opus 5.5 --- web/view/src/githubHost.ts | 61 +++++++++++++++++++++---------- web/view/src/main.ts | 21 ++++++++--- web/view/src/modelCache.test.ts | 65 +++++++++++++++++++++++++++++++++ web/view/src/modelCache.ts | 62 +++++++++++++++++++++++++++++++ 4 files changed, 184 insertions(+), 25 deletions(-) create mode 100644 web/view/src/modelCache.test.ts create mode 100644 web/view/src/modelCache.ts diff --git a/web/view/src/githubHost.ts b/web/view/src/githubHost.ts index 1151acf2a..0e04ef51f 100644 --- a/web/view/src/githubHost.ts +++ b/web/view/src/githubHost.ts @@ -8,6 +8,7 @@ import { GitHub, GitHubError, type Refs, type Tree } from "./github.ts"; import type { Compare, EditResult, Host, NoteKind, SourceEdit } from "./host.ts"; import type { TokenStore } from "./login.ts"; import { parseViewModel, type ViewModel } from "./model.ts"; +import { ModelCache } from "./modelCache.ts"; import { Project } from "./project.ts"; import { planProject, ProjectPlanError } from "./repoProject.ts"; import { formatRoute, parseRoute, type Route } from "./route.ts"; @@ -56,8 +57,8 @@ export class GitHubHost implements Host { private project: Project | null = null; /** The sources as read from GitHub, to return to on reset. */ private pristine = new Map(); - private readonly models = new Map(); - private pristineModels = new Map(); + private readonly models = new ModelCache(); + private pristineModels: ViewModel[] = []; /** Repository directory of the project root, to turn a project file into an address. */ private root = ""; private state: GitHubState; @@ -119,7 +120,7 @@ export class GitHubHost implements Host { this.root = plan.root; this.pristine = new Map(files); this.project = new Project(route.repo, files); - this.models.clear(); + this.models.replace([]); this.modules = []; this.edited = false; this.note("evaluating…", "busy"); @@ -139,14 +140,14 @@ export class GitHubHost implements Host { if ("error" in outcome) continue; for (const listener of this.modelListeners) listener([outcome.model]); } - this.pristineModels = new Map(this.models); + this.pristineModels = this.models.currentModels(); if (baseTree !== null && route.base !== null) { this.note(`reading ${route.base}…`, "busy"); this.compare = await this.readBase(route.base, await baseTree, tree); if (!fresh()) return; this.note(null, "busy"); // The sheets are drawn again with the changes marked. - for (const listener of this.modelListeners) listener([...this.models.values()]); + for (const listener of this.modelListeners) listener(this.models.currentModels()); } this.setState({ ...this.state, loading: false }); } catch (error) { @@ -215,8 +216,8 @@ export class GitHubHost implements Host { } catch (error) { return { error: errorMessage(error) }; } - this.models.set(model.module.id, model); - this.modules = [...this.models.keys()].sort(); + this.models.set(model); + this.modules = this.models.ids(); return { model, note: outcome.valued ? null : `${file} has no values of its own: ${outcome.note}`, @@ -267,7 +268,7 @@ export class GitHubHost implements Host { if (same && this.project) { const module = this.moduleOfPath(next.path); if (module !== null) { - for (const listener of this.openListeners) listener(module); + void this.openModule(module); return; } } @@ -279,7 +280,7 @@ export class GitHubHost implements Host { if (!path.endsWith(".gcl")) return false; const module = this.moduleOfPath(path); if (module !== null) { - for (const listener of this.openListeners) listener(module); + void this.openModule(module); return true; } location.hash = formatRoute({ ...this.route, path }); @@ -302,7 +303,7 @@ export class GitHubHost implements Host { } pathOf(module: string): string | null { - const model = this.models.get(module); + const model = this.models.last(module); if (!model) return null; return this.root === "" ? model.module.file : `${this.root}/${model.module.file}`; } @@ -310,10 +311,29 @@ export class GitHubHost implements Host { private moduleOfPath(path: string): string | null { if (this.root !== "" && !path.startsWith(`${this.root}/`)) return null; const relative = this.root === "" ? path : path.slice(this.root.length + 1); - for (const model of this.models.values()) { - if (model.module.file === relative) return model.module.id; + return this.models.moduleOfFile(relative); + } + + /** The current model of `module`, projected again when an edit left it stale. */ + private async currentModel(module: string): Promise { + const current = this.models.current(module); + if (current) return current; + const last = this.models.last(module); + if (!last) return null; + const generation = this.generation; + const outcome = await this.projectFile(last.module.file); + if (generation !== this.generation) return null; + if ("error" in outcome) { + this.note(outcome.error, "error"); + return null; } - return null; + return outcome.model; + } + + /** Ask the shell to show `module`, as of the current sources. */ + private async openModule(module: string): Promise { + if (!(await this.currentModel(module))) return; + for (const listener of this.openListeners) listener(module); } // ---- refs ---- @@ -347,7 +367,7 @@ export class GitHubHost implements Host { // ---- Host ---- model(module: string): ViewModel | null { - return this.models.get(module) ?? null; + return this.models.current(module); } source(_model: ViewModel, file: string): string | null { @@ -366,7 +386,7 @@ export class GitHubHost implements Host { }; const models: ViewModel[] = []; for (const module of open) { - const shown = this.models.get(module); + const shown = this.models.current(module); if (!shown) continue; let outcome; try { @@ -381,21 +401,22 @@ export class GitHubHost implements Host { return rollback(errorMessage(error)); } } - for (const fresh of models) this.models.set(fresh.module.id, fresh); + // The modules off screen read the edited sources too: their models are stale now. + this.models.edited(models); this.edited = true; return { ok: true, models }; } request(module: string): void { - const model = this.models.get(module); - if (model) for (const listener of this.modelListeners) listener([model]); + void this.currentModel(module).then((model) => { + if (model) for (const listener of this.modelListeners) listener([model]); + }); } reset(): void { if (!this.project) return; this.project = new Project(this.route.repo, this.pristine); - this.models.clear(); - for (const [id, model] of this.pristineModels) this.models.set(id, model); + this.models.replace(this.pristineModels); this.edited = false; } diff --git a/web/view/src/main.ts b/web/view/src/main.ts index 6d6e32405..3c58286fb 100644 --- a/web/view/src/main.ts +++ b/web/view/src/main.ts @@ -645,10 +645,21 @@ function renderTrail(w: Workspace): void { fileEl.append(copyEl, resetEl); } +/** A trail to open once the host has its module's current model (an edit left it stale). */ +let pendingOpen: string[] | null = null; + /** Show `trail`'s last module as the main sheet, with no panes pinned. */ function open(trail: string[]): void { const module = trail.at(-1); - if (module === undefined || !known(module)) return; + if (module === undefined) return; + if (!known(module)) { + if (host.modules.includes(module)) { + pendingOpen = trail; + host.request(module); + } + return; + } + pendingOpen = null; const main = createSheet({ module, instance: null }); ws = { sheets: [main], trail, mode: ws?.mode ?? "fit" }; placePanes(ws); @@ -692,6 +703,8 @@ function pin(key: PaneKey): void { /** Take fresh models from the host: every sheet showing one of them is re-laid out in place. */ function adoptModels(models: ViewModel[]): void { + const opening = pendingOpen; + if (opening !== null && known(opening.at(-1) ?? "")) open(opening); if (!ws) return; let changed = false; for (const model of models) { @@ -1173,10 +1186,8 @@ function externalMenu(w: Workspace, node: ViewNode, event: MouseEvent): void { openMenu(event.clientX, event.clientY, [ { label: `Open ${file}`, - disabled: !known(node.module), - run: () => { - if (known(node.module)) open([...w.trail, node.module]); - }, + disabled: !known(node.module) && !host.modules.includes(node.module), + run: () => open([...w.trail, node.module]), }, pinned ? { label: `Unpin ${name}`, run: () => unpin(pinned) } diff --git a/web/view/src/modelCache.test.ts b/web/view/src/modelCache.test.ts new file mode 100644 index 000000000..a0d3c14e6 --- /dev/null +++ b/web/view/src/modelCache.test.ts @@ -0,0 +1,65 @@ +import { describe, expect, test } from "vite-plus/test"; +import type { ViewModel } from "./model.ts"; +import { ModelCache } from "./modelCache.ts"; + +function model(id: string, revision: number): ViewModel { + return { + version: 0, + module: { id, file: `src/${id}.gcl`, package: "p" }, + indexes: [], + nodes: [ + { + id: "x", + name: "x", + module: id, + kind: "node", + axes: [], + dimension: "Dimensionless", + unit: null, + pub: true, + deps: [], + expr: String(revision), + reduce: null, + instance: null, + source: null, + value: null, + }, + ], + }; +} + +describe("ModelCache", () => { + test("after an edit only the re-projected models are current", () => { + const cache = new ModelCache(); + cache.replace([model("p.a", 1), model("p.b", 1)]); + // `a` was on screen and re-projected; `b`, which reads `a`, was not. + cache.edited([model("p.a", 2)]); + expect(cache.current("p.a")?.nodes[0]?.expr).toBe("2"); + expect(cache.current("p.b")).toBeNull(); + expect(cache.isStale("p.b")).toBe(true); + expect(cache.currentModels().map((m) => m.module.id)).toEqual(["p.a"]); + // The stale model still says where the module is, to project it again. + expect(cache.last("p.b")?.module.file).toBe("src/p.b.gcl"); + expect(cache.moduleOfFile("src/p.b.gcl")).toBe("p.b"); + expect(cache.ids()).toEqual(["p.a", "p.b"]); + }); + + test("projecting a stale module again makes it current", () => { + const cache = new ModelCache(); + cache.replace([model("p.a", 1), model("p.b", 1)]); + cache.edited([]); + cache.set(model("p.b", 2)); + expect(cache.current("p.b")?.nodes[0]?.expr).toBe("2"); + expect(cache.current("p.a")).toBeNull(); + }); + + test("a reset to the models as read makes them all current again", () => { + const cache = new ModelCache(); + const read = [model("p.a", 1), model("p.b", 1)]; + cache.replace(read); + cache.edited([model("p.a", 2)]); + cache.replace(read); + expect(cache.current("p.a")?.nodes[0]?.expr).toBe("1"); + expect(cache.current("p.b")?.nodes[0]?.expr).toBe("1"); + }); +}); diff --git a/web/view/src/modelCache.ts b/web/view/src/modelCache.ts new file mode 100644 index 000000000..46687de81 --- /dev/null +++ b/web/view/src/modelCache.ts @@ -0,0 +1,62 @@ +// The models a host has projected, and which of them still describe the +// sources it holds. An edit re-projects only the modules on screen; every other +// model then describes sources that are gone. Such a stale model is never handed +// out as current: its id and file only locate the module, to project it again. +import type { ViewModel } from "./model.ts"; + +export class ModelCache { + private readonly models = new Map(); + private readonly stale = new Set(); + + /** Keep a model projected from the current sources. */ + set(model: ViewModel): void { + this.models.set(model.module.id, model); + this.stale.delete(model.module.id); + } + + /** The sources changed: `fresh` are projected from them, every other model is stale. */ + edited(fresh: readonly ViewModel[]): void { + for (const id of this.models.keys()) this.stale.add(id); + for (const model of fresh) this.set(model); + } + + /** Start over with `models`, all current: a new project read, or a reset to the sources as read. */ + replace(models: Iterable): void { + this.models.clear(); + this.stale.clear(); + for (const model of models) this.set(model); + } + + /** The model of `module` as of the current sources; null when there is none or it is stale. */ + current(module: string): ViewModel | null { + return this.stale.has(module) ? null : (this.models.get(module) ?? null); + } + + /** Every model as of the current sources. */ + currentModels(): ViewModel[] { + return [...this.models.values()].filter((model) => !this.stale.has(model.module.id)); + } + + /** The last model of `module`, current or stale: where it is, not what it holds. */ + last(module: string): ViewModel | null { + return this.models.get(module) ?? null; + } + + /** Whether `module` has a model, but one from sources since edited. */ + isStale(module: string): boolean { + return this.stale.has(module); + } + + /** Every module with a model, current or stale, sorted. */ + ids(): string[] { + return [...this.models.keys()].sort(); + } + + /** The module whose file is `file`, current or stale. */ + moduleOfFile(file: string): string | null { + for (const model of this.models.values()) { + if (model.module.file === file) return model.module.id; + } + return null; + } +}