diff --git a/docs/dev/deploy.md b/docs/dev/deploy.md index 10e0a6c4f..439ee399f 100644 --- a/docs/dev/deploy.md +++ b/docs/dev/deploy.md @@ -63,6 +63,28 @@ gcloud app services set-traffic default --splits==1 (`--no-promote` flows through to `gcloud app deploy` via the script's `"$@"` passthrough; the script still runs `deploy:clean` afterward via the trap.) +### Canary deploy + Firebase login: `pnpm deploy:canary` + +The `--no-promote` flow above lets you `curl` the canary, but you cannot actually **log in** to it: Firebase OAuth (`signInWithRedirect` via `auth.simlin.com`) rejects any origin not in Firebase's *Authorized domains*, and the canary is reachable only at a versioned `https://-dot-...appspot.com` URL that isn't on that list. So a full end-to-end test (including Google sign-in, the new-user flow, and saving a model) isn't possible on a bare `--no-promote` version. + +[`scripts/deploy-canary.mjs`](/scripts/deploy-canary.mjs) (`pnpm deploy:canary`) closes that gap. It: + +1. Reads the target project (`gcloud config get-value project`; override with `--project ` or `SIMLIN_CANARY_PROJECT`) and prints a warning that it touches **production** Firebase config and deploys (but does **not** promote traffic). +2. Builds + deploys via `scripts/deploy-web-staged.sh --no-promote` (it does **not** pass `--version`, so gcloud auto-generates the version id). +3. Discovers the just-deployed version id (`gcloud app versions list --sort-by=~version.createTime --limit=1`) and its region-aware URL (`gcloud app browse --no-launch-browser --version=`). +4. Adds that version's host to the Identity Toolkit `authorizedDomains` list via a **read-modify-write**: GET the config, append the host to the full existing list, then PATCH it back with `?updateMask=authorizedDomains`. The PATCH replaces the whole repeated field, so it always sends the complete current-plus-new list -- it never wipes `localhost` / `firebaseapp.com` / `app.simlin.com`. It prints the before/after list. +5. Prints the canary URL, the smoke-test checklist, the exact `gcloud app services set-traffic default --splits==1` promote command, and the exact cleanup command. **It does not promote traffic.** + +After testing, clean up: + +```bash +pnpm deploy:canary --cleanup # de-authorize the host + delete the version +``` + +Cleanup is the inverse: it removes the host from `authorizedDomains` (same read-modify-write) and deletes the version (freeing its slot toward the GAE version cap). It deletes rather than `stop`s because production uses `automatic_scaling`, for which `gcloud app versions stop` is rejected. The delete is guarded by the version's current traffic share: if you have already promoted the canary it is now production, so cleanup refuses to delete it (de-authorizing the host only) and tells you to delete the previous, now-idle version instead -- cleanup can never cause an outage. + +This deliberately uses **your own** credentials (`gcloud auth print-access-token`), not the CI deploy service account: mutating Firebase auth config needs `roles/firebaseauth.admin`, an operator-level grant intentionally kept off the CI SA. The deploy/authorize and the traffic promote are separate, explicit steps so traffic is never switched implicitly. Keep the canary host authorized only while you are testing it -- leaving stale appspot hosts in `authorizedDomains` needlessly widens the OAuth surface. + ## What gets uploaded, and what runs on the instance `gcloud app deploy` uploads the whole repo except `.gcloudignore` entries: `node_modules`, `target/`, `test/`, `/build*`, `scripts/`, `.github/`, `website/`, `examples/`, `src/jupyter/`, `src/app/public`, `src/app/build*`, `src/server/public`, `src/server/config`, `src/app/firebase.json`, and `.app.prod.yaml` itself. Not excluded, and load-bearing: diff --git a/package.json b/package.json index 8fd7f8477..c61f33155 100644 --- a/package.json +++ b/package.json @@ -37,6 +37,7 @@ "clean": "pnpm -r run clean && cargo clean", "deploy:web": "bash scripts/deploy-web.sh", "deploy:web:staged": "bash scripts/deploy-web-staged.sh", + "deploy:canary": "node scripts/deploy-canary.mjs", "build:deploy-staging": "node scripts/build-deploy-staging.mjs", "start": "node src/server/lib", "test:scripts": "node --test scripts/tests/*.test.mjs", diff --git a/scripts/deploy-canary.mjs b/scripts/deploy-canary.mjs new file mode 100644 index 000000000..1e77dee48 --- /dev/null +++ b/scripts/deploy-canary.mjs @@ -0,0 +1,575 @@ +#!/usr/bin/env node +// +// Deploy HEAD to Google App Engine as a NON-TRAFFIC canary, then authorize the +// canary's versioned host in Firebase so the operator can exercise the REAL +// product end-to-end -- including Google sign-in -- before switching 100% of +// production traffic. +// +// Why this script exists: +// - `gcloud app deploy --no-promote` creates a version reachable only at a +// versioned `*.appspot.com` URL (e.g. https://-dot-...). +// That host is NOT in Firebase's Authorized domains list, so Firebase OAuth +// (signInWithRedirect via auth.simlin.com) rejects the calling origin and the +// operator cannot actually log in to test the canary. +// - The fix is to add the canary's host to the Identity Toolkit +// `authorizedDomains` list for the duration of the test, then remove it. +// +// This deliberately uses the OPERATOR's own credentials (`gcloud auth +// print-access-token`), NOT the CI deploy service account: mutating Firebase +// auth config requires roles/firebaseauth.admin, which is an operator-level +// grant we intentionally keep off the CI deploy SA. +// +// It does NOT promote traffic. Promoting is a separate, explicit command the +// script prints for you to run after the smoke test passes. +// +// Usage: +// node scripts/deploy-canary.mjs [--project ] # deploy canary + authorize +// node scripts/deploy-canary.mjs --cleanup [--project ] +// +// The build/deploy itself is delegated to scripts/deploy-web-staged.sh (the +// self-contained staged deploy); this script adds only the --no-promote canary +// orchestration + Firebase authorized-domain management around it. +// +// --------------------------------------------------------------------------- +// The file is split into a PURE CORE (exported, unit-tested in +// scripts/tests/deploy-canary.test.mjs) and an IMPERATIVE SHELL (all the +// gcloud/fetch side effects). main() runs only when the file is executed +// directly, so importing it for tests has no side effects. +// --------------------------------------------------------------------------- + +import { spawnSync } from 'node:child_process'; +import path from 'node:path'; +import { fileURLToPath, pathToFileURL } from 'node:url'; + +const SCRIPT_DIR = path.dirname(fileURLToPath(import.meta.url)); +const REPO_ROOT = path.resolve(SCRIPT_DIR, '..'); + +const IDENTITY_TOOLKIT_BASE = 'https://identitytoolkit.googleapis.com/admin/v2'; + +// =========================================================================== +// PURE CORE -- no I/O, deterministic, unit-tested. +// =========================================================================== + +/** + * Reduce a URL (or already-bare host) to its bare hostname: scheme, path, + * query, and any port are stripped. Firebase `authorizedDomains` entries are + * bare hostnames (the OAuth check compares window.location.hostname), so the + * port is intentionally dropped. Accepts input without a scheme so a host can + * be passed straight through. Throws on empty/non-string input. + */ +export function hostFromUrl(input) { + if (typeof input !== 'string' || input.trim() === '') { + throw new Error('hostFromUrl: expected a non-empty URL or host string'); + } + const trimmed = input.trim(); + // Give the URL parser a scheme to chew on when one is absent, so a bare host + // like "foo.appspot.com" parses instead of throwing. + const hasScheme = /^[a-zA-Z][a-zA-Z0-9+.-]*:\/\//.test(trimmed); + const url = new URL(hasScheme ? trimmed : `https://${trimmed}`); + return url.hostname; +} + +/** + * Return a new authorized-domains list that includes `host`, deduped and with + * existing order preserved. A no-op (returns an equivalent deduped copy) when + * `host` is already present. Never mutates the input. + * + * This is the read side of the read-modify-write that guarantees the PATCH + * never wipes the existing list: the caller passes the full current list (from + * a successful GET); this appends to it rather than replacing it. + */ +export function addAuthorizedDomain(domains, host) { + const base = Array.isArray(domains) ? domains : []; + const seen = new Set(); + const out = []; + for (const d of [...base, host]) { + if (!seen.has(d)) { + seen.add(d); + out.push(d); + } + } + return out; +} + +/** + * Return a new authorized-domains list with every occurrence of `host` + * removed. A no-op (returns a copy) when `host` is absent. Never mutates the + * input. Like addAuthorizedDomain, this operates on the FULL current list so + * the resulting PATCH replaces the field without losing other entries. + */ +export function removeAuthorizedDomain(domains, host) { + const base = Array.isArray(domains) ? domains : []; + return base.filter((d) => d !== host); +} + +/** First non-blank, trimmed line of text (or '' for blank/non-string input). */ +export function firstNonEmptyLine(text) { + if (typeof text !== 'string') return ''; + for (const line of text.split('\n')) { + const t = line.trim(); + if (t !== '') return t; + } + return ''; +} + +/** + * Extract the first http(s) URL from arbitrary command output. `gcloud app + * browse --no-launch-browser` prints a human sentence followed by the URL, so + * we cannot just trim the whole output. Returns undefined when none is found. + */ +export function extractUrl(text) { + if (typeof text !== 'string') return undefined; + const m = text.match(/https?:\/\/\S+/); + return m ? m[0] : undefined; +} + +/** + * Fraction of `default`-service traffic currently allocated to `versionId`, + * read from the `split.allocations` map (versionId -> fraction) returned by + * `gcloud app services describe`. Returns 0 when the version is absent or the + * map is missing/malformed, and treats a non-finite-number allocation as 0 + * (never truthy) so the caller's serving check can't be fooled by a string. + * + * Cleanup uses this to REFUSE to delete a version that is serving traffic: once + * the operator promotes the canary, it IS production, and deleting it is a full + * outage. De-authorizing the host is always safe; only the delete is dangerous. + */ +export function versionTrafficShare(allocations, versionId) { + if (!allocations || typeof allocations !== 'object') return 0; + const share = allocations[versionId]; + return typeof share === 'number' && Number.isFinite(share) ? share : 0; +} + +/** + * Parse argv (without node/script) into { mode, project, version }. + * mode: 'deploy' (default) | 'cleanup' | 'help' + * Supports `--flag value` and `--flag=value`. Throws on unknown flags and on + * --cleanup without a version id (failing loud beats a confusing later error). + */ +export function parseArgs(argv) { + const args = { mode: 'deploy', project: undefined, version: undefined }; + const list = [...argv]; + while (list.length > 0) { + const token = list.shift(); + const eq = token.startsWith('--') ? token.indexOf('=') : -1; + const flag = eq >= 0 ? token.slice(0, eq) : token; + const inlineVal = eq >= 0 ? token.slice(eq + 1) : undefined; + // Do not consume a following flag as this flag's value: `--cleanup + // --project p` must leave version unset (and report the missing version), + // not swallow `--project`. + const takeVal = () => { + if (inlineVal !== undefined) return inlineVal; + return list.length > 0 && !list[0].startsWith('--') ? list.shift() : undefined; + }; + switch (flag) { + case '--cleanup': + args.mode = 'cleanup'; + args.version = takeVal(); + break; + case '--project': { + // Fail loud on a missing/blank value: this tool deploys and mutates + // PROD Firebase config, so `--project $EMPTY` (or `--project --cleanup`) + // must error rather than silently fall back to the gcloud default and + // operate on the wrong project. Omitting --project entirely is still + // fine -- that intentionally uses the env var / gcloud default. + const value = takeVal(); + if (value === undefined || value.trim() === '') { + throw new Error('--project requires a value, e.g. --project my-gcp-project'); + } + args.project = value; + break; + } + case '--help': + case '-h': + args.mode = 'help'; + break; + default: + throw new Error(`unknown argument: ${token}`); + } + } + if (args.mode === 'cleanup' && (!args.version || args.version.startsWith('--'))) { + throw new Error('--cleanup requires a version id, e.g. --cleanup 20260627t123456'); + } + return args; +} + +// =========================================================================== +// IMPERATIVE SHELL -- gcloud + fetch side effects. Kept deliberately small and +// obvious; the testable logic lives in the pure core above. +// =========================================================================== + +function die(msg) { + console.error(`\nERROR: ${msg}`); + process.exit(1); +} + +/** + * Run a command, inheriting stdio by default so the operator watches progress. + * With { capture: true } stdout is returned as a string (stderr still streams). + * Fails fast with a clear message on a missing binary or non-zero exit. + */ +function run(cmd, cmdArgs, { capture = false } = {}) { + const res = spawnSync(cmd, cmdArgs, { + cwd: REPO_ROOT, + encoding: 'utf8', + stdio: capture ? ['inherit', 'pipe', 'inherit'] : 'inherit', + }); + if (res.error) { + if (res.error.code === 'ENOENT') { + const hint = cmd === 'gcloud' ? ' Install the Google Cloud SDK and authenticate: gcloud auth login.' : ''; + die(`'${cmd}' was not found on PATH.${hint}`); + } + die(`failed to run ${cmd}: ${res.error.message}`); + } + if (res.status !== 0) { + die(`\`${cmd} ${cmdArgs.join(' ')}\` exited with status ${res.status}`); + } + return capture ? res.stdout : ''; +} + +/** Resolve the GCP project: explicit override wins, else the gcloud default. */ +function resolveProject(override) { + if (override) return override; + const project = firstNonEmptyLine(run('gcloud', ['config', 'get-value', 'project'], { capture: true })); + if (!project || project === '(unset)') { + die('no gcloud project configured. Pass --project or run: gcloud config set project '); + } + return project; +} + +/** The operator's own OAuth access token (NOT the CI deploy SA's). */ +function accessToken() { + const token = firstNonEmptyLine(run('gcloud', ['auth', 'print-access-token'], { capture: true })); + if (!token) { + die('could not obtain an access token. Run: gcloud auth login'); + } + return token; +} + +/** Build + deploy the staged server with --no-promote (traffic stays put). */ +function deployStagedNoPromote(project) { + console.log('\n==> Building and deploying the staged server with --no-promote (no traffic switch)\n'); + // Reuse the proven staged build/deploy orchestration; do NOT pass --version, + // so gcloud auto-generates the version id we then discover below. Pass the + // resolved --project through (deploy-web-staged.sh forwards "$@" to + // `gcloud app deploy`): otherwise an operator who selected a non-default + // project via --project/SIMLIN_CANARY_PROJECT would deploy HEAD to the + // AMBIENT gcloud project while the version lookup and Firebase mutation below + // target the selected one -- a split-brain that could deploy to the wrong + // project. resolveProject() always yields a concrete id, so this is also + // correct (and harmlessly explicit) in the default case. + run('bash', [path.join(REPO_ROOT, 'scripts/deploy-web-staged.sh'), '--no-promote', `--project=${project}`]); +} + +/** + * The id of the most-recently-created `default` version. We query by createTime + * immediately after our deploy rather than scraping the deploy output (gcloud's + * human output format is not a stable contract). Tradeoff: if another deploy of + * the same service races this one, the newest version could be theirs -- this + * is a single-operator manual canary tool, so that race is acceptable; the + * printed URL lets the operator sanity-check the id before promoting. + */ +function latestVersionId(project) { + const out = run( + 'gcloud', + [ + 'app', + 'versions', + 'list', + '--service=default', + '--sort-by=~version.createTime', + '--limit=1', + '--format=value(id)', + `--project=${project}`, + ], + { capture: true }, + ); + const id = firstNonEmptyLine(out); + if (!id) { + die('could not determine the deployed version id from `gcloud app versions list`'); + } + return id; +} + +/** + * The canary's URL + bare host, via `gcloud app browse` (region-aware -- we do + * NOT hand-construct the appspot host, since the region segment varies). + */ +function versionUrlAndHost(project, id) { + const out = run( + 'gcloud', + ['app', 'browse', '--no-launch-browser', '--service=default', `--version=${id}`, `--project=${project}`], + { capture: true }, + ); + const url = extractUrl(out); + if (!url) { + die(`could not extract the canary URL from \`gcloud app browse\` output:\n${out}`); + } + return { url, host: hostFromUrl(url) }; +} + +/** + * Delete an idle canary version (cleanup). We delete rather than `stop` because + * `gcloud app versions stop` only works for manual/basic scaling, and prod uses + * automatic_scaling (app.yaml) -- a stop would error out, leaving the version + * (and its slot toward the GAE version cap) behind. delete works for any scaling + * and reclaims the slot. The caller guarantees this version serves no traffic + * (gcloud also refuses to delete a version that is receiving traffic). gcloud + * prompts for confirmation; stdio is inherited so the operator confirms. + */ +function deleteVersion(project, id) { + run('gcloud', ['app', 'versions', 'delete', id, '--service=default', `--project=${project}`]); +} + +/** + * The `default` service's current traffic split as a { versionId: fraction } + * map. Read from `gcloud app services describe` so cleanup can avoid deleting a + * version that is serving traffic. Returns {} if the service has no split yet. + */ +function serviceTrafficAllocations(project) { + const out = run('gcloud', ['app', 'services', 'describe', 'default', '--format=json', `--project=${project}`], { + capture: true, + }); + let parsed; + try { + parsed = JSON.parse(out); + } catch { + die(`could not parse \`gcloud app services describe default\` JSON output:\n${out}`); + } + return parsed?.split?.allocations ?? {}; +} + +/** + * GET the project's Identity Toolkit config. This MUST succeed before any + * PATCH: the read-modify-write below depends on a real current list, never an + * assumed-empty one. Returns the parsed config object. + */ +async function getIdentityConfig(project, token) { + const res = await fetch(`${IDENTITY_TOOLKIT_BASE}/projects/${encodeURIComponent(project)}/config`, { + headers: { authorization: `Bearer ${token}` }, + }); + if (!res.ok) { + const body = await res.text(); + const hint = + res.status === 403 + ? ' (does your account have roles/firebaseauth.admin? this needs the operator creds, not the CI deploy SA)' + : ''; + die(`Identity Toolkit GET config failed: ${res.status} ${res.statusText}${hint}\n${body}`); + } + // Parse defensively: a 200 whose body isn't JSON is a real (if rare) failure + // we want to surface clearly, not a confusing low-level throw. + const text = await res.text(); + try { + return JSON.parse(text); + } catch { + die(`Identity Toolkit GET config returned ${res.status} with a non-JSON body:\n${text}`); + } +} + +/** + * PATCH authorizedDomains with `?updateMask=authorizedDomains`. The mask scopes + * the write to exactly that field, but the API REPLACES the whole repeated + * field with the body's list -- so `domains` MUST be the full desired list + * (current entries + the change), which the pure add/remove helpers produce + * from a freshly GET-ed current list. Sending only the new host here would wipe + * localhost / firebaseapp.com / app.simlin.com. + */ +async function patchAuthorizedDomains(project, token, domains) { + const res = await fetch( + `${IDENTITY_TOOLKIT_BASE}/projects/${encodeURIComponent(project)}/config?updateMask=authorizedDomains`, + { + method: 'PATCH', + headers: { authorization: `Bearer ${token}`, 'content-type': 'application/json' }, + body: JSON.stringify({ authorizedDomains: domains }), + }, + ); + if (!res.ok) { + const body = await res.text(); + die(`Identity Toolkit PATCH config failed: ${res.status} ${res.statusText}\n${body}`); + } + // The response body is discarded, so do NOT parse it: a successful PATCH whose + // body isn't JSON must not be reported as a failure AFTER the change applied. +} + +function printDomains(label, domains) { + console.log(` ${label}:`); + for (const d of domains) { + console.log(` - ${d}`); + } + if (domains.length === 0) { + console.log(' (none)'); + } +} + +function printHelp() { + console.log( + [ + 'Deploy a NON-TRAFFIC canary to App Engine and authorize its host in Firebase.', + '', + 'Usage:', + ' node scripts/deploy-canary.mjs [--project ]', + ' Build + deploy HEAD with --no-promote, then add the canary host to', + ' Firebase authorizedDomains so you can log in and smoke-test it.', + '', + ' node scripts/deploy-canary.mjs --cleanup [--project ]', + ' Remove that version host from authorizedDomains, then delete the version', + ' UNLESS it is serving traffic (a promoted canary is production; the delete', + ' is refused so cleanup can never cause an outage).', + '', + 'Project defaults to `gcloud config get-value project`; override with', + '--project or the SIMLIN_CANARY_PROJECT env var. Mutating Firebase auth', + 'config requires roles/firebaseauth.admin on YOUR account (this uses your', + 'own gcloud credentials, not the CI deploy service account). Traffic is', + 'never promoted; the deploy + promote are separate steps.', + ].join('\n'), + ); +} + +async function runDeployMode(project, token) { + // Preflight: validate Firebase access BEFORE the (long) deploy, so a missing + // firebaseauth.admin grant fails in seconds rather than after an upload. + console.log('==> Preflight: reading current Firebase authorized domains'); + const preflight = await getIdentityConfig(project, token); + printDomains('current authorizedDomains', preflight.authorizedDomains ?? []); + + deployStagedNoPromote(project); + + const versionId = latestVersionId(project); + const { url, host } = versionUrlAndHost(project, versionId); + console.log(`\n==> Canary version: ${versionId}`); + console.log(` Canary URL: ${url}`); + console.log(` Canary host: ${host}`); + + // Read-modify-write against a FRESH GET (the config could have changed during + // the deploy), then PATCH the full merged list. + console.log('\n==> Authorizing the canary host in Firebase'); + const current = await getIdentityConfig(project, token); + const before = current.authorizedDomains ?? []; + const after = addAuthorizedDomain(before, host); + printDomains('before', before); + if (after.length === before.length) { + console.log(` ${host} is already authorized -- no change.`); + } else { + await patchAuthorizedDomains(project, token, after); + printDomains('after', after); + } + + printPostDeploy(project, versionId, url); +} + +function printPostDeploy(project, versionId, url) { + const projectFlag = `--project=${project}`; + console.log( + [ + '', + '============================================================', + 'Canary is deployed and authorized -- traffic NOT switched.', + '============================================================', + '', + `Canary URL: ${url}`, + '', + 'Smoke test the canary (against the URL above):', + ` - curl -sI ${url}/ -> 200 HTML, links a hashed /static/js/index..js`, + ` - curl -sI ${url}/static/js/sd-component.js -> 200 (the embed component)`, + ` - curl -sI ${url}/static/wasm/.module.wasm -> 200, content-type: application/wasm`, + ' - In a browser: LOG IN WITH GOOGLE (this is what the authorized-domain step enables),', + ' land on Home with no console errors.', + ' - Open an example model and confirm it simulates; edit + save + reload persists.', + '', + 'When the smoke test PASSES, switch 100% of traffic to the canary:', + ` gcloud app services set-traffic default --splits=${versionId}=1 ${projectFlag}`, + '', + 'Cleanup -- two cases:', + ` (a) ABANDONING the canary (did NOT promote): de-authorizes the host AND deletes`, + ' the version, which is safe because it serves no traffic:', + ` node scripts/deploy-canary.mjs --cleanup ${versionId} ${projectFlag}`, + ' (b) AFTER PROMOTING: this version is now production. The same cleanup command', + ' is safe to run -- it will de-authorize the host but REFUSE to delete a', + ' serving version. To reclaim resources, delete the PREVIOUS (now-idle)', + ' version instead, not this one:', + ` gcloud app versions list --service=default ${projectFlag}`, + ` gcloud app versions delete --service=default ${projectFlag}`, + '', + 'NOTE: keep the canary host authorized only as long as you are testing it;', + 'leaving stale appspot hosts in authorizedDomains widens the OAuth surface.', + ].join('\n'), + ); +} + +async function runCleanupMode(project, token, versionId) { + // Derive the host the same way the deploy path did (browse is region-aware). + // Do this BEFORE deleting the version so browse can still resolve it. + const { host } = versionUrlAndHost(project, versionId); + console.log(`==> Cleaning up canary version ${versionId} (host ${host})`); + + console.log('\n==> Removing the canary host from Firebase authorized domains'); + const current = await getIdentityConfig(project, token); + const before = current.authorizedDomains ?? []; + const after = removeAuthorizedDomain(before, host); + printDomains('before', before); + if (after.length === before.length) { + console.log(` ${host} was not authorized -- no change.`); + } else { + await patchAuthorizedDomains(project, token, after); + printDomains('after', after); + } + + // Guard the delete: if the operator promoted this canary, it is now serving + // production traffic and deleting it is a full-site outage. De-authorizing the + // host above is always safe; only the delete is dangerous, so we refuse it + // here rather than blindly running it. + const share = versionTrafficShare(serviceTrafficAllocations(project), versionId); + if (share > 0) { + const pct = (share * 100).toFixed(share < 0.01 ? 2 : 0); + console.log( + [ + '', + `REFUSING TO DELETE version ${versionId}: it is serving ${pct}% of default-service traffic.`, + 'De-authorized the host only. If you promoted the canary, it IS production now --', + 'deleting it would take the site down. To reclaim resources, delete the PREVIOUS', + '(now-idle) version instead:', + ` gcloud app versions list --service=default --project=${project}`, + ` gcloud app versions delete --service=default --project=${project}`, + ].join('\n'), + ); + return; + } + + // delete (not stop): prod uses automatic_scaling, where `gcloud app versions + // stop` is rejected; delete works and frees the slot toward the version cap. + console.log('\n==> Deleting the canary version (it is serving no traffic)'); + deleteVersion(project, versionId); + console.log(`\nDone. Version ${versionId} is deleted and its host is de-authorized.`); +} + +async function main() { + const args = parseArgs(process.argv.slice(2)); + if (args.mode === 'help') { + printHelp(); + return; + } + + const project = resolveProject(args.project ?? process.env.SIMLIN_CANARY_PROJECT); + + console.log('============================================================'); + console.log(`Project: ${project}`); + console.log('WARNING: this touches PRODUCTION Firebase auth config and'); + console.log('deploys to production App Engine. It does NOT promote traffic.'); + console.log('============================================================\n'); + + const token = accessToken(); + + if (args.mode === 'cleanup') { + await runCleanupMode(project, token, args.version); + } else { + await runDeployMode(project, token); + } +} + +// Run only when executed directly, so importing the pure helpers in tests has +// no side effects (mirrors the guard-free build-deploy-staging.mjs by gating +// the entrypoint instead). +const invokedDirectly = process.argv[1] && import.meta.url === pathToFileURL(process.argv[1]).href; +if (invokedDirectly) { + main().catch((err) => die(err.message)); +} diff --git a/scripts/deploy-staging-manifests.mjs b/scripts/deploy-staging-manifests.mjs index 3711a88b4..2baab6b8a 100644 --- a/scripts/deploy-staging-manifests.mjs +++ b/scripts/deploy-staging-manifests.mjs @@ -81,7 +81,9 @@ function rewriteDepBlock(deps, fileRef, context) { * * @param serverPkg parsed src/server/package.json * @param opts.vendorDir relative dir the workspace packages are vendored under (default ./vendor) - * @param opts.packageManager optional packageManager string to pin (e.g. "pnpm@10.6.0") + * @param opts.packageManager optional packageManager string to pin (e.g. "pnpm@10.6.0"). + * When it is a `pnpm@` spec, engines.pnpm is also pinned to that + * version (the GAE buildpack reads engines.pnpm, not corepack/packageManager). */ export function buildStagingServerManifest(serverPkg, opts = {}) { const { vendorDir = './vendor', packageManager } = opts; @@ -114,8 +116,29 @@ export function buildStagingServerManifest(serverPkg, opts = {}) { manifest.optionalDependencies = optionalDependencies; } + // Carry forward any engines the server declared, then pin engines.pnpm from + // the packageManager version. App Engine's Node buildpack reads engines.pnpm + // and gives it precedence over packageManager/corepack (implemented in + // GoogleCloudPlatform/buildpacks pnpm.go/registry.go), where an exact version + // short-circuits registry resolution to exactly that pnpm. Without it the + // buildpack may fall back to its bundled pnpm and re-resolve or reject our + // frozen lockfile, failing the deploy. The explicit pin wins over any + // inherited pnpm range so the buildpack uses exactly the locked version. + // packageManager is still emitted verbatim (corepack wants the full spec incl. + // the +sha512 hash); the bare version is required here because a +hash + // constraint would miss the exact-semver fast path. + const engines = { ...(serverPkg.engines ?? {}) }; if (packageManager) { manifest.packageManager = packageManager; + // packageManager spec: `pnpm@` optionally with a `+` build + // suffix (e.g. `pnpm@10.6.0+sha512.`). engines wants the bare version. + const m = /^pnpm@([^+\s]+)/.exec(packageManager.trim()); + if (m) { + engines.pnpm = m[1]; + } + } + if (Object.keys(engines).length > 0) { + manifest.engines = engines; } return manifest; } diff --git a/scripts/tests/deploy-canary.test.mjs b/scripts/tests/deploy-canary.test.mjs new file mode 100644 index 000000000..ea9950c76 --- /dev/null +++ b/scripts/tests/deploy-canary.test.mjs @@ -0,0 +1,213 @@ +import assert from 'node:assert/strict'; +import { describe, it } from 'node:test'; + +import { + hostFromUrl, + addAuthorizedDomain, + removeAuthorizedDomain, + firstNonEmptyLine, + extractUrl, + parseArgs, + versionTrafficShare, +} from '../deploy-canary.mjs'; + +describe('hostFromUrl', () => { + it('strips scheme and trailing slash', () => { + assert.equal(hostFromUrl('https://foo.appspot.com/'), 'foo.appspot.com'); + }); + + it('accepts http as well as https', () => { + assert.equal(hostFromUrl('http://foo.appspot.com'), 'foo.appspot.com'); + }); + + it('strips a path and query', () => { + assert.equal( + hostFromUrl('https://20260627t123456-dot-myproj.uc.r.appspot.com/some/path?q=1'), + '20260627t123456-dot-myproj.uc.r.appspot.com', + ); + }); + + it('drops the port (authorizedDomains entries are bare hostnames)', () => { + assert.equal(hostFromUrl('http://localhost:5000/'), 'localhost'); + }); + + it('accepts a bare host with no scheme', () => { + assert.equal(hostFromUrl('foo-dot-bar.appspot.com'), 'foo-dot-bar.appspot.com'); + }); + + it('trims surrounding whitespace', () => { + assert.equal(hostFromUrl(' https://foo.appspot.com/ '), 'foo.appspot.com'); + }); + + it('throws on empty / non-string input', () => { + assert.throws(() => hostFromUrl('')); + assert.throws(() => hostFromUrl(undefined)); + }); +}); + +describe('addAuthorizedDomain', () => { + it('appends a new host', () => { + assert.deepEqual(addAuthorizedDomain(['localhost', 'app.simlin.com'], 'v1-dot-p.appspot.com'), [ + 'localhost', + 'app.simlin.com', + 'v1-dot-p.appspot.com', + ]); + }); + + it('is a no-op when the host is already present (idempotent)', () => { + const domains = ['localhost', 'app.simlin.com']; + assert.deepEqual(addAuthorizedDomain(domains, 'app.simlin.com'), ['localhost', 'app.simlin.com']); + }); + + it('dedups pre-existing duplicates while preserving order', () => { + assert.deepEqual(addAuthorizedDomain(['a', 'a', 'b'], 'c'), ['a', 'b', 'c']); + }); + + it('does not mutate the input list', () => { + const domains = ['localhost']; + addAuthorizedDomain(domains, 'new.example.com'); + assert.deepEqual(domains, ['localhost']); + }); + + it('tolerates an undefined/empty current list', () => { + assert.deepEqual(addAuthorizedDomain(undefined, 'x.appspot.com'), ['x.appspot.com']); + assert.deepEqual(addAuthorizedDomain([], 'x.appspot.com'), ['x.appspot.com']); + }); +}); + +describe('removeAuthorizedDomain', () => { + it('removes the host', () => { + assert.deepEqual(removeAuthorizedDomain(['localhost', 'v1-dot-p.appspot.com'], 'v1-dot-p.appspot.com'), [ + 'localhost', + ]); + }); + + it('is a no-op when the host is absent (idempotent)', () => { + assert.deepEqual(removeAuthorizedDomain(['localhost'], 'nope.appspot.com'), ['localhost']); + }); + + it('removes every occurrence if duplicated', () => { + assert.deepEqual(removeAuthorizedDomain(['a', 'b', 'a'], 'a'), ['b']); + }); + + it('does not mutate the input list', () => { + const domains = ['localhost', 'x.appspot.com']; + removeAuthorizedDomain(domains, 'x.appspot.com'); + assert.deepEqual(domains, ['localhost', 'x.appspot.com']); + }); + + it('tolerates an undefined/empty current list', () => { + assert.deepEqual(removeAuthorizedDomain(undefined, 'x'), []); + assert.deepEqual(removeAuthorizedDomain([], 'x'), []); + }); +}); + +describe('firstNonEmptyLine', () => { + it('returns the first non-blank trimmed line', () => { + assert.equal(firstNonEmptyLine('\n \n 20260627t1\n more\n'), '20260627t1'); + }); + + it('trims a single value', () => { + assert.equal(firstNonEmptyLine(' myproject \n'), 'myproject'); + }); + + it('returns empty string for blank / non-string input', () => { + assert.equal(firstNonEmptyLine(' \n '), ''); + assert.equal(firstNonEmptyLine(undefined), ''); + }); +}); + +describe('extractUrl', () => { + it('pulls the first http(s) URL out of noisy output', () => { + const out = + 'Did not detect your browser. Go to this link to view your app:\n' + + 'https://20260627t1-dot-myproj.uc.r.appspot.com\n'; + assert.equal(extractUrl(out), 'https://20260627t1-dot-myproj.uc.r.appspot.com'); + }); + + it('handles a plain URL', () => { + assert.equal(extractUrl('https://foo.appspot.com/'), 'https://foo.appspot.com/'); + }); + + it('returns undefined when there is no URL', () => { + assert.equal(extractUrl('no url here'), undefined); + assert.equal(extractUrl(undefined), undefined); + }); +}); + +describe('parseArgs', () => { + it('defaults to deploy mode', () => { + assert.deepEqual(parseArgs([]), { mode: 'deploy', project: undefined, version: undefined }); + }); + + it('parses --project with a separate value', () => { + assert.equal(parseArgs(['--project', 'myproj']).project, 'myproj'); + }); + + it('parses --project=value inline', () => { + assert.equal(parseArgs(['--project=myproj']).project, 'myproj'); + }); + + it('parses --cleanup into cleanup mode', () => { + const args = parseArgs(['--cleanup', '20260627t1']); + assert.equal(args.mode, 'cleanup'); + assert.equal(args.version, '20260627t1'); + }); + + it('parses --cleanup= inline', () => { + assert.equal(parseArgs(['--cleanup=20260627t1']).version, '20260627t1'); + }); + + it('combines --cleanup and --project', () => { + const args = parseArgs(['--cleanup', '20260627t1', '--project', 'p']); + assert.equal(args.mode, 'cleanup'); + assert.equal(args.version, '20260627t1'); + assert.equal(args.project, 'p'); + }); + + it('maps --help/-h to help mode', () => { + assert.equal(parseArgs(['--help']).mode, 'help'); + assert.equal(parseArgs(['-h']).mode, 'help'); + }); + + it('throws when --cleanup has no version', () => { + assert.throws(() => parseArgs(['--cleanup']), /requires a version/); + assert.throws(() => parseArgs(['--cleanup', '--project', 'p']), /requires a version/); + }); + + it('throws on an unknown argument', () => { + assert.throws(() => parseArgs(['--bogus']), /unknown argument/); + }); + + it('throws when --project has no value', () => { + // A prod-mutating tool must not silently fall back to the default project + // when an override was clearly intended (e.g. `--project $EMPTY`). + assert.throws(() => parseArgs(['--project']), /--project requires a value/); + assert.throws(() => parseArgs(['--project', '--cleanup', 'v']), /--project requires a value/); + assert.throws(() => parseArgs(['--project', '']), /--project requires a value/); + assert.throws(() => parseArgs(['--project=']), /--project requires a value/); + assert.throws(() => parseArgs(['--project= ']), /--project requires a value/); + }); +}); + +describe('versionTrafficShare', () => { + it('returns the fraction allocated to the version', () => { + assert.equal(versionTrafficShare({ '20260627t1': 1 }, '20260627t1'), 1); + assert.equal(versionTrafficShare({ a: 0.3, b: 0.7 }, 'b'), 0.7); + }); + + it('returns 0 when the version is absent (safe to stop)', () => { + assert.equal(versionTrafficShare({ other: 1 }, '20260627t1'), 0); + }); + + it('returns 0 for a missing/empty allocations map', () => { + assert.equal(versionTrafficShare(undefined, 'v'), 0); + assert.equal(versionTrafficShare(null, 'v'), 0); + assert.equal(versionTrafficShare({}, 'v'), 0); + }); + + it('treats a non-numeric allocation as 0 rather than truthy', () => { + assert.equal(versionTrafficShare({ v: '1' }, 'v'), 0); + assert.equal(versionTrafficShare({ v: NaN }, 'v'), 0); + }); +}); diff --git a/scripts/tests/deploy-staging-manifests.test.mjs b/scripts/tests/deploy-staging-manifests.test.mjs index 54b891967..18e5d3684 100644 --- a/scripts/tests/deploy-staging-manifests.test.mjs +++ b/scripts/tests/deploy-staging-manifests.test.mjs @@ -69,6 +69,42 @@ describe('buildStagingServerManifest', () => { assert.equal(buildStagingServerManifest(serverPkg).packageManager, undefined); }); + it('pins engines.pnpm from the packageManager version (the GAE buildpack reads engines, not corepack)', () => { + const out = buildStagingServerManifest(serverPkg, { packageManager: 'pnpm@10.6.0' }); + assert.equal(out.engines.pnpm, '10.6.0'); + }); + + it('strips the +sha512 build-metadata suffix when deriving engines.pnpm', () => { + const out = buildStagingServerManifest(serverPkg, { + packageManager: 'pnpm@10.6.0+sha512.abc123def456', + }); + assert.equal(out.engines.pnpm, '10.6.0'); + // packageManager itself is preserved verbatim (corepack still wants the hash). + assert.equal(out.packageManager, 'pnpm@10.6.0+sha512.abc123def456'); + }); + + it('adds no engines when no packageManager is provided and the server declares none', () => { + assert.equal(buildStagingServerManifest(serverPkg).engines, undefined); + }); + + it('preserves the server-declared engines when no packageManager is provided', () => { + const withEngines = { ...serverPkg, engines: { node: '>=20' } }; + const out = buildStagingServerManifest(withEngines); + assert.deepEqual(out.engines, { node: '>=20' }); + }); + + it('merges the pnpm pin into the server-declared engines, with the pin winning', () => { + const withEngines = { ...serverPkg, engines: { node: '>=20', pnpm: '>=9' } }; + const out = buildStagingServerManifest(withEngines, { packageManager: 'pnpm@10.6.0' }); + assert.deepEqual(out.engines, { node: '>=20', pnpm: '10.6.0' }); + }); + + it('adds no engines.pnpm for a non-pnpm packageManager, but keeps packageManager', () => { + const out = buildStagingServerManifest(serverPkg, { packageManager: 'yarn@4.1.0' }); + assert.equal(out.packageManager, 'yarn@4.1.0'); + assert.equal(out.engines, undefined); + }); + it('produces a manifest with no residual workspace: protocol', () => { const out = buildStagingServerManifest(serverPkg); // Should not throw. diff --git a/src/core/datamodel.ts b/src/core/datamodel.ts index e4249c42f..9b0a855ed 100644 --- a/src/core/datamodel.ts +++ b/src/core/datamodel.ts @@ -22,6 +22,7 @@ import { type JsonGraphicalFunctionScale, type JsonArrayedEquation, type JsonElementEquation, + type JsonDataSource, type JsonView, type JsonViewElement, type JsonStockViewElement, @@ -146,6 +147,56 @@ export function graphicalFunctionToJson(gf: GraphicalFunction): JsonGraphicalFun return result; } +// DataSource: external-data reference (Vensim GET DIRECT DATA / CONSTANTS / +// LOOKUPS / SUBSCRIPT). Carried on a variable's compat so an edit to any other +// field does not drop the reference. The wire shape and `kind` values are +// defined by the Rust json::JsonDataSource serializer (src/simlin-engine/src/json.rs). + +export type DataSourceKind = 'data' | 'constants' | 'lookups' | 'subscript'; + +export interface DataSource { + readonly kind: DataSourceKind; + readonly file: string; + readonly tabOrDelimiter: string; + readonly rowOrCol: string; + readonly cell: string; +} + +function dataSourceKindFromJson(kind: string): DataSourceKind { + // Mirror the Rust data_source_from_json fallback: any unrecognized kind + // (including the default "data") maps to 'data'. + switch (kind) { + case 'constants': + return 'constants'; + case 'lookups': + return 'lookups'; + case 'subscript': + return 'subscript'; + default: + return 'data'; + } +} + +export function dataSourceFromJson(json: JsonDataSource): DataSource { + return { + kind: dataSourceKindFromJson(json.kind), + file: json.file ?? '', + tabOrDelimiter: json.tabOrDelimiter ?? '', + rowOrCol: json.rowOrCol ?? '', + cell: json.cell ?? '', + }; +} + +export function dataSourceToJson(ds: DataSource): JsonDataSource { + return { + kind: ds.kind, + file: ds.file, + tabOrDelimiter: ds.tabOrDelimiter, + rowOrCol: ds.rowOrCol, + cell: ds.cell, + }; +} + // Equation types export interface ScalarEquation { @@ -159,32 +210,103 @@ export interface ApplyToAllEquation { readonly equation: string; } +// A single element of an arrayed (per-element) equation. Carries the +// element's equation plus any per-element graphical function and ACTIVE +// INITIAL equation (the only per-element compat field the engine round-trips). +export interface ArrayedElement { + readonly equation: string; + readonly graphicalFunction: GraphicalFunction | undefined; + readonly activeInitial: string | undefined; +} + export interface ArrayedEquation { readonly type: 'arrayed'; readonly dimensionNames: readonly string[]; - readonly elements: ReadonlyMap; + readonly elements: ReadonlyMap; + // The EXCEPT default equation applied to elements not listed in `elements`, + // and the flag indicating it is an EXCEPT default. `hasExceptDefault` is only + // meaningful when `defaultEquation` is set. + readonly defaultEquation: string | undefined; + readonly hasExceptDefault: boolean; } export type Equation = ScalarEquation | ApplyToAllEquation | ArrayedEquation; +// Build the datamodel ArrayedEquation from its JSON form, preserving per-element +// equations, graphical functions, and ACTIVE INITIAL equations as well as the +// EXCEPT default. Shared by stocks, flows, and auxes (the per-element shape is +// identical across variable kinds). +export function arrayedEquationFromJson(json: JsonArrayedEquation): ArrayedEquation { + const dimensionNames: readonly string[] = json.dimensions ?? []; + const elements = new Map( + (json.elements ?? []).map( + (el: JsonElementEquation) => + [ + el.subscript, + { + equation: el.equation, + graphicalFunction: el.graphicalFunction ? graphicalFunctionFromJson(el.graphicalFunction) : undefined, + // The engine only round-trips activeInitial out of a per-element compat. + activeInitial: el.compat?.activeInitial, + }, + ] as [string, ArrayedElement], + ), + ); + // Mirror the engine: a legacy payload without hasExceptDefault infers `true` + // when a default equation is present (preserving pre-flag behavior). + const hasExceptDefault = json.hasExceptDefault ?? json.equation !== undefined; + return { + type: 'arrayed', + dimensionNames, + elements, + defaultEquation: json.equation, + hasExceptDefault, + }; +} + +// Serialize a datamodel ArrayedEquation back to JSON. Only emits the EXCEPT +// default flag when a default equation is present (mirroring the engine, where +// the flag is meaningless without one). +export function arrayedEquationToJson(eq: ArrayedEquation): JsonArrayedEquation { + const result: JsonArrayedEquation = { + dimensions: [...eq.dimensionNames], + elements: [...eq.elements].map(([subscript, el]) => { + const jsonEl: JsonElementEquation = { subscript, equation: el.equation }; + if (el.graphicalFunction) { + jsonEl.graphicalFunction = graphicalFunctionToJson(el.graphicalFunction); + } + if (el.activeInitial) { + jsonEl.compat = { activeInitial: el.activeInitial }; + } + return jsonEl; + }), + }; + if (eq.defaultEquation !== undefined) { + result.equation = eq.defaultEquation; + result.hasExceptDefault = eq.hasExceptDefault; + } + return result; +} + function stockEquationFromJson( initialEquation: string | undefined, arrayedEquation: JsonArrayedEquation | undefined, ): Equation { if (arrayedEquation) { - const dimensionNames: readonly string[] = arrayedEquation.dimensions ?? []; - if (arrayedEquation.elements && arrayedEquation.elements.length > 0) { - return { - type: 'arrayed', - dimensionNames, - elements: new Map( - arrayedEquation.elements.map((el: JsonElementEquation) => [el.subscript, el.equation] as [string, string]), - ), - }; + // The engine distinguishes ApplyToAll from Arrayed by the PRESENCE of the + // `elements` field, not by it being non-empty (json.rs: ApplyToAll omits + // `elements`; Arrayed always emits it, even as []). An Arrayed with no + // explicit elements + an EXCEPT default + hasExceptDefault:false means every + // element is missing and evaluates to 0, NOT the default (compiler/mod.rs: + // a missing element uses the default only when apply_default_for_missing is + // true). Collapsing that to applyToAll would silently change behavior, so + // route on presence: `elements` present (even []) => arrayed. + if (arrayedEquation.elements !== undefined) { + return arrayedEquationFromJson(arrayedEquation); } else { return { type: 'applyToAll', - dimensionNames, + dimensionNames: arrayedEquation.dimensions ?? [], equation: arrayedEquation.equation ?? '', }; } @@ -203,23 +325,18 @@ function auxEquationFromJson( } if (arrayedEquation) { - const dimensionNames: readonly string[] = arrayedEquation.dimensions ?? []; - if (arrayedEquation.elements && arrayedEquation.elements.length > 0) { - return { - equation: { - type: 'arrayed', - dimensionNames, - elements: new Map( - arrayedEquation.elements.map((el: JsonElementEquation) => [el.subscript, el.equation] as [string, string]), - ), - }, - graphicalFunction, - }; + // Route on the PRESENCE of `elements`, not on it being non-empty -- see the + // matching note in stockEquationFromJson. The engine emits `elements` for + // every Arrayed (even []) and omits it for ApplyToAll; an Arrayed with no + // explicit elements + hasExceptDefault:false has every element evaluate to 0, + // which collapsing to applyToAll would silently change. + if (arrayedEquation.elements !== undefined) { + return { equation: arrayedEquationFromJson(arrayedEquation), graphicalFunction }; } else { return { equation: { type: 'applyToAll', - dimensionNames, + dimensionNames: arrayedEquation.dimensions ?? [], equation: arrayedEquation.equation ?? '', }, graphicalFunction, @@ -243,12 +360,7 @@ function stockEquationToJson(equation: Equation): { initialEquation?: string; ar }, }; } else if (equation.type === 'arrayed') { - return { - arrayedEquation: { - dimensions: [...equation.dimensionNames], - elements: [...equation.elements].map(([subscript, eqn]) => ({ subscript, equation: eqn })), - }, - }; + return { arrayedEquation: arrayedEquationToJson(equation) }; } return {}; } @@ -264,12 +376,7 @@ function auxEquationToJson(equation: Equation): { equation?: string; arrayedEqua }, }; } else if (equation.type === 'arrayed') { - return { - arrayedEquation: { - dimensions: [...equation.dimensionNames], - elements: [...equation.elements].map(([subscript, eqn]) => ({ subscript, equation: eqn })), - }, - }; + return { arrayedEquation: arrayedEquationToJson(equation) }; } return {}; } @@ -287,6 +394,10 @@ export interface Stock { readonly nonNegative: boolean; readonly canBeModuleInput: boolean; readonly isPublic: boolean; + // Vensim ACTIVE INITIAL: the variable's separate initialization equation. + readonly activeInitial: string | undefined; + // External-data reference (Vensim GET DIRECT DATA/CONSTANTS/LOOKUPS/SUBSCRIPT). + readonly dataSource: DataSource | undefined; readonly data: Readonly> | undefined; readonly errors: readonly EquationError[] | undefined; readonly unitErrors: readonly UnitError[] | undefined; @@ -303,6 +414,8 @@ export interface Flow { readonly nonNegative: boolean; readonly canBeModuleInput: boolean; readonly isPublic: boolean; + readonly activeInitial: string | undefined; + readonly dataSource: DataSource | undefined; readonly data: Readonly> | undefined; readonly errors: readonly EquationError[] | undefined; readonly unitErrors: readonly UnitError[] | undefined; @@ -318,6 +431,8 @@ export interface Aux { readonly gf: GraphicalFunction | undefined; readonly canBeModuleInput: boolean; readonly isPublic: boolean; + readonly activeInitial: string | undefined; + readonly dataSource: DataSource | undefined; readonly data: Readonly> | undefined; readonly errors: readonly EquationError[] | undefined; readonly unitErrors: readonly UnitError[] | undefined; @@ -344,6 +459,12 @@ export interface Module { readonly documentation: string; readonly units: string; readonly references: readonly ModuleReference[]; + // The engine reads only canBeModuleInput, isPublic, and dataSource out of a + // module's compat (From in json.rs uses defaults for the rest), so + // ACTIVE INITIAL and nonNegative are intentionally absent here. + readonly canBeModuleInput: boolean; + readonly isPublic: boolean; + readonly dataSource: DataSource | undefined; readonly data: Readonly> | undefined; readonly errors: readonly EquationError[] | undefined; readonly unitErrors: readonly UnitError[] | undefined; @@ -382,9 +503,14 @@ export function stockFromJson(json: JsonStock): Stock { outflows: json.outflows ?? [], // OR-merge: old code never writes compat booleans and new code never // writes top-level booleans, so both cannot be meaningfully set at once. + // Mirrors the engine's JSON reader (json.rs), which OR-merges all three + // legacy top-level flags so a project saved in the old schema is not + // silently stripped on the next edit. nonNegative: json.compat?.nonNegative || json.nonNegative || false, - canBeModuleInput: json.compat?.canBeModuleInput ?? false, - isPublic: json.compat?.isPublic ?? false, + canBeModuleInput: json.compat?.canBeModuleInput || json.canBeModuleInput || false, + isPublic: json.compat?.isPublic || json.isPublic || false, + activeInitial: json.compat?.activeInitial, + dataSource: json.compat?.dataSource ? dataSourceFromJson(json.compat.dataSource) : undefined, data: undefined, errors: undefined, unitErrors: undefined, @@ -429,6 +555,18 @@ export function stockToJson(stock: Stock): JsonStock { } result.compat.isPublic = true; } + if (stock.activeInitial) { + if (!result.compat) { + result.compat = {}; + } + result.compat.activeInitial = stock.activeInitial; + } + if (stock.dataSource) { + if (!result.compat) { + result.compat = {}; + } + result.compat.dataSource = dataSourceToJson(stock.dataSource); + } if (stock.documentation) { result.documentation = stock.documentation; } @@ -450,9 +588,16 @@ export function flowFromJson(json: JsonFlow): Flow { gf: graphicalFunction, // OR-merge: old code never writes compat booleans and new code never // writes top-level booleans, so both cannot be meaningfully set at once. + // Mirrors the engine's JSON reader (json.rs), which OR-merges all three + // legacy top-level flags so a project saved in the old schema is not + // silently stripped on the next edit. nonNegative: json.compat?.nonNegative || json.nonNegative || false, - canBeModuleInput: json.compat?.canBeModuleInput ?? false, - isPublic: json.compat?.isPublic ?? false, + canBeModuleInput: json.compat?.canBeModuleInput || json.canBeModuleInput || false, + isPublic: json.compat?.isPublic || json.isPublic || false, + // ACTIVE INITIAL: top-level compat wins, else fall back to the arrayed + // equation's compat (a legacy/native JSON shape the engine reader accepts). + activeInitial: json.compat?.activeInitial || json.arrayedEquation?.compat?.activeInitial, + dataSource: json.compat?.dataSource ? dataSourceFromJson(json.compat.dataSource) : undefined, data: undefined, errors: undefined, unitErrors: undefined, @@ -498,6 +643,18 @@ export function flowToJson(flow: Flow): JsonFlow { } result.compat.isPublic = true; } + if (flow.activeInitial) { + if (!result.compat) { + result.compat = {}; + } + result.compat.activeInitial = flow.activeInitial; + } + if (flow.dataSource) { + if (!result.compat) { + result.compat = {}; + } + result.compat.dataSource = dataSourceToJson(flow.dataSource); + } if (flow.documentation) { result.documentation = flow.documentation; } @@ -517,8 +674,14 @@ export function auxFromJson(json: JsonAuxiliary): Aux { documentation: json.documentation ?? '', units: json.units ?? '', gf: graphicalFunction, - canBeModuleInput: json.compat?.canBeModuleInput ?? false, - isPublic: json.compat?.isPublic ?? false, + // OR-merge legacy top-level flags with compat, mirroring the engine's JSON + // reader (json.rs); old JSON wrote them at top level, new JSON under compat. + canBeModuleInput: json.compat?.canBeModuleInput || json.canBeModuleInput || false, + isPublic: json.compat?.isPublic || json.isPublic || false, + // ACTIVE INITIAL: top-level compat wins, else fall back to the arrayed + // equation's compat (a legacy/native JSON shape the engine reader accepts). + activeInitial: json.compat?.activeInitial || json.arrayedEquation?.compat?.activeInitial, + dataSource: json.compat?.dataSource ? dataSourceFromJson(json.compat.dataSource) : undefined, data: undefined, errors: undefined, unitErrors: undefined, @@ -558,6 +721,18 @@ export function auxToJson(aux: Aux): JsonAuxiliary { } result.compat.isPublic = true; } + if (aux.activeInitial) { + if (!result.compat) { + result.compat = {}; + } + result.compat.activeInitial = aux.activeInitial; + } + if (aux.dataSource) { + if (!result.compat) { + result.compat = {}; + } + result.compat.dataSource = dataSourceToJson(aux.dataSource); + } if (aux.documentation) { result.documentation = aux.documentation; } @@ -572,6 +747,11 @@ export function moduleFromJson(json: JsonModule): Module { documentation: json.documentation ?? '', units: json.units ?? '', references: (json.references ?? []).map((ref: JsonModuleReference) => moduleReferenceFromJson(ref)), + // OR-merge legacy top-level flags with compat, mirroring the engine's JSON + // reader (json.rs); old JSON wrote them at top level, new JSON under compat. + canBeModuleInput: json.compat?.canBeModuleInput || json.canBeModuleInput || false, + isPublic: json.compat?.isPublic || json.isPublic || false, + dataSource: json.compat?.dataSource ? dataSourceFromJson(json.compat.dataSource) : undefined, data: undefined, errors: undefined, unitErrors: undefined, @@ -593,6 +773,24 @@ export function moduleToJson(mod: Module): JsonModule { if (mod.units) { result.units = mod.units; } + if (mod.canBeModuleInput) { + if (!result.compat) { + result.compat = {}; + } + result.compat.canBeModuleInput = true; + } + if (mod.isPublic) { + if (!result.compat) { + result.compat = {}; + } + result.compat.isPublic = true; + } + if (mod.dataSource) { + if (!result.compat) { + result.compat = {}; + } + result.compat.dataSource = dataSourceToJson(mod.dataSource); + } if (mod.documentation) { result.documentation = mod.documentation; } diff --git a/src/core/jest.config.js b/src/core/jest.config.js index 3001c4d78..5e51aeb1b 100644 --- a/src/core/jest.config.js +++ b/src/core/jest.config.js @@ -9,7 +9,11 @@ const config = { testMatch: ['/tests/**/*.test.ts'], moduleFileExtensions: ['ts', 'js'], moduleNameMapper: { + // Mirror @simlin/engine's own exports aliases so a core test can drive the + // engine at runtime: these internal subpaths resolve to platform-specific + // node files that do not live under the literal subpath. '^@simlin/engine/internal/wasm$': '/../engine/lib/internal/wasm.node.js', + '^@simlin/engine/internal/backend-factory$': '/../engine/lib/backend-factory.node.js', '^@simlin/engine/(.*)$': '/../engine/lib/$1.js', '^@simlin/engine$': '/../engine/lib/index.js', }, diff --git a/src/core/tests/datamodel-roundtrip-e2e.test.ts b/src/core/tests/datamodel-roundtrip-e2e.test.ts new file mode 100644 index 000000000..aee079bd0 --- /dev/null +++ b/src/core/tests/datamodel-roundtrip-e2e.test.ts @@ -0,0 +1,284 @@ +// Copyright 2026 The Simlin Authors. All rights reserved. +// Use of this source code is governed by the Apache License, +// Version 2.0, that can be found in the LICENSE file. + +// End-to-end regression for silent variable data-loss through the REAL engine. +// +// A pure fromJson<->toJson test (datamodel.test.ts) can pass while still using +// a wrong JSON key name, because both halves agree on the typo. These tests +// instead drive the actual Rust serializer (src/simlin-engine/src/json.rs) via +// the WASM engine, so a key name that does not match the serializer is caught: +// the engine drops the unknown field and the survival assertion fails. +// +// The scenario mirrors the editor's persistence path exactly. The editor loads +// a serialized project, builds the in-memory datamodel, and -- on any edit -- +// re-serializes a variable as a FULL upsert (replace-by-UID, see +// src/simlin-engine/src/patch.rs). Any wire field datamodel.ts fails to +// round-trip is therefore dropped the moment the user edits an unrelated field. + +import * as fs from 'fs'; +import * as path from 'path'; + +import type { JsonProjectPatch } from '@simlin/engine'; + +import { projectFromJson, projectToJson, auxToJson, stockToJson, moduleToJson } from '../datamodel'; + +// This e2e drives the REAL WASM engine, so it needs `pnpm build` to have +// produced the engine package (lib/) and libsimlin.wasm. Keep `pnpm --filter +// @simlin/core test` runnable standalone on a clean checkout by SKIPPING (not +// failing) when the built engine is absent; CI and the pre-commit hook build +// first, so this integration coverage always runs there. The @simlin/engine +// VALUE import is dynamic (inside loadWasm) so a missing build does not break +// module resolution for the skipped suite -- the pure datamodel.test.ts still +// covers every field without the engine. +const wasmPath = path.join(__dirname, '..', '..', 'engine', 'core', 'libsimlin.wasm'); +const engineBuilt = fs.existsSync(wasmPath); +const describeIfEngine = engineBuilt ? describe : describe.skip; +if (!engineBuilt) { + console.warn( + `[datamodel-roundtrip-e2e] skipping engine-backed checks: ${wasmPath} not found; run \`pnpm build\` to exercise them.`, + ); +} + +type EngineModule = typeof import('@simlin/engine'); +let engine: EngineModule; + +async function loadWasm(): Promise { + engine = await import('@simlin/engine'); + const wasmBuffer = fs.readFileSync(wasmPath); + await engine.resetWasm(); + engine.configureWasm({ source: wasmBuffer }); + await engine.ready(); +} + +// A project whose variables carry every field that datamodel.ts previously +// dropped. Key names are camelCase to match the Rust serializer. +const BASE_PROJECT = JSON.stringify({ + name: 'roundtrip', + simSpecs: { startTime: 0, endTime: 10, dt: '1' }, + dimensions: [{ name: 'region', elements: ['north', 'south'] }], + models: [ + { + name: 'main', + stocks: [ + { + name: 'level', + inflows: [], + outflows: [], + initialEquation: '0', + // [1] ACTIVE INITIAL on a stock. + compat: { activeInitial: '5' }, + }, + ], + flows: [], + auxiliaries: [ + { + name: 'imported', + equation: '0', + // [5] external-data reference (GET DIRECT DATA). + compat: { + dataSource: { + kind: 'data', + file: 'data.xlsx', + tabOrDelimiter: 'Sheet1', + rowOrCol: 'A', + cell: 'B2', + }, + }, + }, + { + name: 'arrayed', + arrayedEquation: { + dimensions: ['region'], + // [3] EXCEPT default equation + flag. + equation: '99', + hasExceptDefault: true, + elements: [ + { + subscript: 'north', + equation: '1', + // [4] per-element graphical function + per-element ACTIVE INITIAL. + graphicalFunction: { yPoints: [0, 1, 2], xScale: { min: 0, max: 2 }, yScale: { min: 0, max: 2 } }, + compat: { activeInitial: '3' }, + }, + ], + }, + }, + ], + modules: [ + { + name: 'sub_inst', + modelName: 'sub', + // Module compat: the engine reads canBeModuleInput, isPublic, dataSource. + compat: { + canBeModuleInput: true, + isPublic: true, + dataSource: { + kind: 'constants', + file: 'c.csv', + tabOrDelimiter: ',', + rowOrCol: '1', + cell: 'A1', + }, + }, + }, + ], + }, + { + name: 'sub', + stocks: [], + flows: [], + auxiliaries: [{ name: 'out', equation: '1' }], + }, + ], +}); + +interface ParsedAux { + name: string; + compat?: { activeInitial?: string; dataSource?: Record }; + arrayedEquation?: { + equation?: string; + hasExceptDefault?: boolean; + elements?: { + subscript: string; + equation: string; + compat?: { activeInitial?: string }; + graphicalFunction?: { yPoints?: number[] }; + }[]; + }; +} +interface ParsedStock { + name: string; + compat?: { activeInitial?: string }; +} +interface ParsedModule { + name: string; + compat?: { canBeModuleInput?: boolean; isPublic?: boolean; dataSource?: Record }; +} +interface ParsedModel { + name: string; + stocks: ParsedStock[]; + auxiliaries: ParsedAux[]; + modules?: ParsedModule[]; +} +interface ParsedProject { + models: ParsedModel[]; +} + +async function serializeProject(json: string): Promise { + const project = await engine.Project.openJson(json); + try { + return JSON.parse(await project.serializeJson()) as ParsedProject; + } finally { + await project.dispose(); + } +} + +function findAux(parsed: ParsedProject, name: string): ParsedAux { + const aux = parsed.models[0].auxiliaries.find((a) => a.name === name); + if (!aux) { + throw new Error(`aux ${name} not found`); + } + return aux; +} + +function assertAllFieldsPresent(parsed: ParsedProject): void { + const stock = parsed.models[0].stocks.find((s) => s.name === 'level'); + expect(stock?.compat?.activeInitial).toBe('5'); + + const imported = findAux(parsed, 'imported'); + expect(imported.compat?.dataSource).toEqual({ + kind: 'data', + file: 'data.xlsx', + tabOrDelimiter: 'Sheet1', + rowOrCol: 'A', + cell: 'B2', + }); + + const arrayed = findAux(parsed, 'arrayed'); + expect(arrayed.arrayedEquation?.equation).toBe('99'); + expect(arrayed.arrayedEquation?.hasExceptDefault).toBe(true); + const north = arrayed.arrayedEquation?.elements?.find((e) => e.subscript === 'north'); + expect(north?.graphicalFunction?.yPoints).toEqual([0, 1, 2]); + expect(north?.compat?.activeInitial).toBe('3'); + + const module = parsed.models[0].modules?.find((m) => m.name === 'sub_inst'); + expect(module?.compat?.canBeModuleInput).toBe(true); + expect(module?.compat?.isPublic).toBe(true); + expect(module?.compat?.dataSource).toEqual({ + kind: 'constants', + file: 'c.csv', + tabOrDelimiter: ',', + rowOrCol: '1', + cell: 'A1', + }); +} + +describeIfEngine('datamodel round-trip through the real engine serializer', () => { + beforeAll(async () => { + await loadWasm(); + }); + + it('the engine preserves the fields the editor must round-trip (validates key names)', async () => { + // Sanity: the serializer keeps every field when the JSON uses these keys. + assertAllFieldsPresent(await serializeProject(BASE_PROJECT)); + }); + + it('survives a datamodel fromJson -> toJson cycle re-fed to the engine', async () => { + const engineJson = await serializeProject(BASE_PROJECT); + + // The exact editor in-memory transform: engine JSON -> datamodel -> JSON. + const datamodel = projectFromJson(engineJson as never); + const rebuiltJson = JSON.stringify(projectToJson(datamodel)); + + // Re-open the datamodel-produced JSON in the engine and re-serialize: a key + // datamodel.ts got wrong on read OR write would be gone by now. + assertAllFieldsPresent(await serializeProject(rebuiltJson)); + }); + + it('survives a full upsert replace (the literal data-loss bug)', async () => { + const project = await engine.Project.openJson(BASE_PROJECT); + try { + const datamodel = projectFromJson(JSON.parse(await project.serializeJson()) as never); + const model = datamodel.models.get('main'); + if (!model) { + throw new Error('main model missing'); + } + + const level = model.variables.get('level'); + const imported = model.variables.get('imported'); + const arrayed = model.variables.get('arrayed'); + const subInst = model.variables.get('sub_inst'); + if ( + level?.type !== 'stock' || + imported?.type !== 'aux' || + arrayed?.type !== 'aux' || + subInst?.type !== 'module' + ) { + throw new Error('expected variables missing'); + } + + // Rebuild each variable as a full upsert -- exactly what the editor sends + // when the user edits any single field. allowErrors so model-validity + // diagnostics never mask the survival assertion. + const patch: JsonProjectPatch = { + models: [ + { + name: 'main', + ops: [ + { type: 'upsertStock', payload: { stock: stockToJson(level) } }, + { type: 'upsertAux', payload: { aux: auxToJson(imported) } }, + { type: 'upsertAux', payload: { aux: auxToJson(arrayed) } }, + { type: 'upsertModule', payload: { module: moduleToJson(subInst) } }, + ], + }, + ], + }; + await project.applyPatch(patch, { allowErrors: true }); + + assertAllFieldsPresent(JSON.parse(await project.serializeJson()) as ParsedProject); + } finally { + await project.dispose(); + } + }); +}); diff --git a/src/core/tests/datamodel.test.ts b/src/core/tests/datamodel.test.ts index 5d5dff23d..de631714d 100644 --- a/src/core/tests/datamodel.test.ts +++ b/src/core/tests/datamodel.test.ts @@ -48,6 +48,8 @@ import type { ScalarEquation, ApplyToAllEquation, ArrayedEquation, + ArrayedElement, + DataSource, SimSpecs, Dimension, Source, @@ -68,6 +70,9 @@ import type { JsonFlowViewElement, JsonLinkViewElement, JsonCloudViewElement, + JsonStock, + JsonAuxiliary, + JsonModule, } from '@simlin/engine'; import { defined, type Series } from '../common'; @@ -141,6 +146,8 @@ describe('Stock', () => { nonNegative: true, canBeModuleInput: false, isPublic: false, + activeInitial: undefined, + dataSource: undefined, data: undefined, errors: undefined, unitErrors: undefined, @@ -190,6 +197,81 @@ describe('Stock', () => { }); }); +describe('legacy top-level canBeModuleInput / isPublic', () => { + // The engine's JSON reader (json.rs) OR-merges legacy top-level + // canBeModuleInput/isPublic with compat for stock/flow/aux/module (read from + // old JSON, never written). datamodel.ts must mirror it so a project saved in + // the old Go `sd` JSON schema -- flags at the top level rather than under + // compat -- does not silently lose module-input / public visibility the next + // time the variable is edited and re-upserted. + it('stockFromJson reads legacy top-level flags', () => { + const stock = stockFromJson({ name: 'level', inflows: [], outflows: [], canBeModuleInput: true, isPublic: true }); + expect(stock.canBeModuleInput).toBe(true); + expect(stock.isPublic).toBe(true); + }); + + it('flowFromJson reads legacy top-level flags', () => { + const flow = flowFromJson({ name: 'rate', canBeModuleInput: true, isPublic: true }); + expect(flow.canBeModuleInput).toBe(true); + expect(flow.isPublic).toBe(true); + }); + + it('auxFromJson reads legacy top-level flags', () => { + const aux = auxFromJson({ name: 'x', canBeModuleInput: true, isPublic: true }); + expect(aux.canBeModuleInput).toBe(true); + expect(aux.isPublic).toBe(true); + }); + + it('moduleFromJson reads legacy top-level flags', () => { + const mod = moduleFromJson({ name: 'sub_inst', modelName: 'sub', canBeModuleInput: true, isPublic: true }); + expect(mod.canBeModuleInput).toBe(true); + expect(mod.isPublic).toBe(true); + }); + + it('moduleFromJson still reads the new compat format', () => { + const mod = moduleFromJson({ + name: 'sub_inst', + modelName: 'sub', + compat: { canBeModuleInput: true, isPublic: true }, + }); + expect(mod.canBeModuleInput).toBe(true); + expect(mod.isPublic).toBe(true); + }); +}); + +describe('ACTIVE INITIAL under arrayedEquation.compat', () => { + // The engine's JSON reader (json.rs) reads a flow/aux ACTIVE INITIAL from the + // top-level compat first, then falls back to arrayedEquation.compat (a legacy/ + // native JSON shape). datamodel.ts must mirror it so an arrayed flow/aux that + // stored ACTIVE INITIAL on the arrayed equation does not lose it on the next + // edit+upsert. (Stocks have no such fallback in the engine reader, so we do + // not add one here.) + it('flowFromJson falls back to arrayedEquation.compat.activeInitial', () => { + const flow = flowFromJson({ + name: 'rate', + arrayedEquation: { dimensions: ['region'], equation: '1', compat: { activeInitial: '7' } }, + }); + expect(flow.activeInitial).toBe('7'); + }); + + it('auxFromJson falls back to arrayedEquation.compat.activeInitial', () => { + const aux = auxFromJson({ + name: 'x', + arrayedEquation: { dimensions: ['region'], equation: '1', compat: { activeInitial: '9' } }, + }); + expect(aux.activeInitial).toBe('9'); + }); + + it('top-level compat.activeInitial wins over the arrayed fallback', () => { + const aux = auxFromJson({ + name: 'x', + compat: { activeInitial: 'top' }, + arrayedEquation: { dimensions: ['region'], equation: '1', compat: { activeInitial: 'arrayed' } }, + }); + expect(aux.activeInitial).toBe('top'); + }); +}); + describe('Flow', () => { it('should roundtrip correctly', () => { const flow: Flow = { @@ -202,6 +284,8 @@ describe('Flow', () => { nonNegative: true, canBeModuleInput: false, isPublic: false, + activeInitial: undefined, + dataSource: undefined, data: undefined, errors: undefined, unitErrors: undefined, @@ -239,6 +323,8 @@ describe('Aux', () => { gf: undefined, canBeModuleInput: false, isPublic: false, + activeInitial: undefined, + dataSource: undefined, data: undefined, errors: undefined, unitErrors: undefined, @@ -271,6 +357,8 @@ describe('Aux', () => { gf, canBeModuleInput: false, isPublic: false, + activeInitial: undefined, + dataSource: undefined, data: undefined, errors: undefined, unitErrors: undefined, @@ -296,6 +384,9 @@ describe('Module', () => { documentation: 'A sector submodel', units: '', references: [{ src: 'input_var', dst: 'sector_input' }], + canBeModuleInput: false, + isPublic: false, + dataSource: undefined, data: undefined, errors: undefined, unitErrors: undefined, @@ -327,6 +418,9 @@ describe('Module', () => { documentation: '', units: '', references: [], + canBeModuleInput: false, + isPublic: false, + dataSource: undefined, data: undefined, errors: undefined, unitErrors: undefined, @@ -356,6 +450,9 @@ describe('Module', () => { { src: '', dst: '' }, { src: 'food', dst: '' }, ], + canBeModuleInput: false, + isPublic: false, + dataSource: undefined, data: undefined, errors: undefined, unitErrors: undefined, @@ -380,6 +477,9 @@ describe('Module', () => { documentation: '', units: '', references: [{ src: 'food', dst: 'sector·sector_input' }], + canBeModuleInput: false, + isPublic: false, + dataSource: undefined, data: undefined, errors: undefined, unitErrors: undefined, @@ -389,6 +489,30 @@ describe('Module', () => { const restored = moduleFromJson(moduleToJson(mod)); expect(restored.references[0].dst).toBe('sector·sector_input'); }); + + it('round-trips module compat (canBeModuleInput, isPublic, dataSource)', () => { + // The engine's From (json.rs) reads these three compat fields and + // defaults the rest, so a module edit must preserve exactly these. + const json: JsonModule = { + name: 'sector', + modelName: 'SectorModel', + compat: { + canBeModuleInput: true, + isPublic: true, + dataSource: { kind: 'constants', file: 'c.csv', tabOrDelimiter: ',', rowOrCol: '1', cell: 'A1' }, + }, + }; + const restored = moduleFromJson(json); + expect(restored.canBeModuleInput).toBe(true); + expect(restored.isPublic).toBe(true); + expect(restored.dataSource?.kind).toBe('constants'); + expect(restored.dataSource?.file).toBe('c.csv'); + + const out = moduleToJson(restored); + expect(out.compat?.canBeModuleInput).toBe(true); + expect(out.compat?.isPublic).toBe(true); + expect(out.compat?.dataSource).toEqual(json.compat!.dataSource); + }); }); describe('View Elements', () => { @@ -837,6 +961,8 @@ describe('canBeModuleInput and isPublic', () => { nonNegative: false, canBeModuleInput: true, isPublic: true, + activeInitial: undefined, + dataSource: undefined, data: undefined, errors: undefined, unitErrors: undefined, @@ -859,6 +985,8 @@ describe('canBeModuleInput and isPublic', () => { nonNegative: false, canBeModuleInput: false, isPublic: false, + activeInitial: undefined, + dataSource: undefined, data: undefined, errors: undefined, unitErrors: undefined, @@ -880,6 +1008,8 @@ describe('canBeModuleInput and isPublic', () => { nonNegative: true, canBeModuleInput: true, isPublic: false, + activeInitial: undefined, + dataSource: undefined, data: undefined, errors: undefined, unitErrors: undefined, @@ -924,6 +1054,8 @@ describe('canBeModuleInput and isPublic', () => { nonNegative: false, canBeModuleInput: true, isPublic: true, + activeInitial: undefined, + dataSource: undefined, data: undefined, errors: undefined, unitErrors: undefined, @@ -945,6 +1077,8 @@ describe('canBeModuleInput and isPublic', () => { nonNegative: true, canBeModuleInput: false, isPublic: true, + activeInitial: undefined, + dataSource: undefined, data: undefined, errors: undefined, unitErrors: undefined, @@ -988,6 +1122,8 @@ describe('canBeModuleInput and isPublic', () => { gf: undefined, canBeModuleInput: true, isPublic: true, + activeInitial: undefined, + dataSource: undefined, data: undefined, errors: undefined, unitErrors: undefined, @@ -1008,6 +1144,8 @@ describe('canBeModuleInput and isPublic', () => { gf: undefined, canBeModuleInput: false, isPublic: false, + activeInitial: undefined, + dataSource: undefined, data: undefined, errors: undefined, unitErrors: undefined, @@ -1035,6 +1173,8 @@ describe('Arrayed Equations', () => { nonNegative: false, canBeModuleInput: false, isPublic: false, + activeInitial: undefined, + dataSource: undefined, data: undefined, errors: undefined, unitErrors: undefined, @@ -1061,16 +1201,20 @@ describe('Arrayed Equations', () => { equation: { type: 'arrayed', dimensionNames: ['regions'], - elements: new Map([ - ['north', '50'], - ['south', '75'], + elements: new Map([ + ['north', { equation: '50', graphicalFunction: undefined, activeInitial: undefined }], + ['south', { equation: '75', graphicalFunction: undefined, activeInitial: undefined }], ]), + defaultEquation: undefined, + hasExceptDefault: false, }, documentation: 'Demand by region', units: '', gf: undefined, canBeModuleInput: false, isPublic: false, + activeInitial: undefined, + dataSource: undefined, data: undefined, errors: undefined, unitErrors: undefined, @@ -1087,8 +1231,63 @@ describe('Arrayed Equations', () => { const eq = restored.equation as ArrayedEquation; expect(eq.dimensionNames).toEqual(['regions']); expect(eq.elements.size).toBe(2); - expect(eq.elements.get('north')).toBe('50'); - expect(eq.elements.get('south')).toBe('75'); + expect(eq.elements.get('north')?.equation).toBe('50'); + expect(eq.elements.get('south')?.equation).toBe('75'); + }); + + // The engine serializes ApplyToAll with NO `elements` field and Arrayed with + // `elements` PRESENT (even []). An Arrayed with elements:[] + a default + + // hasExceptDefault:false means EVERY element is missing and evaluates to 0 (a + // missing element uses the default only when apply_default_for_missing/ + // hasExceptDefault is true -- compiler/mod.rs -- else 0.0). Collapsing such a + // payload to applyToAll would make every element use the default instead: a + // real behavior change the next editor upsert would persist. So the read path + // must route on `elements` PRESENCE, not on it being non-empty. + it('keeps a zero-element arrayed aux arrayed (EXCEPT-excludes-all), not applyToAll', () => { + const restored = auxFromJson({ + name: 'demand', + arrayedEquation: { + dimensions: ['regions'], + equation: '99', + elements: [], + hasExceptDefault: false, + }, + }); + expect(restored.equation.type).toBe('arrayed'); + const eq = restored.equation as ArrayedEquation; + expect(eq.dimensionNames).toEqual(['regions']); + expect(eq.elements.size).toBe(0); + expect(eq.defaultEquation).toBe('99'); + expect(eq.hasExceptDefault).toBe(false); + + // Round-trips back to elements:[] + equation:'99' + hasExceptDefault:false, + // which the engine reads as Arrayed(.., [], Some('99'), false). + const out = auxToJson(restored); + expect(out.arrayedEquation).toBeDefined(); + expect(out.arrayedEquation!.elements).toEqual([]); + expect(out.arrayedEquation!.equation).toBe('99'); + expect(out.arrayedEquation!.hasExceptDefault).toBe(false); + }); + + it('keeps a zero-element arrayed stock arrayed (EXCEPT-excludes-all), not applyToAll', () => { + const restored = stockFromJson({ + name: 'level', + initialEquation: '', + inflows: [], + outflows: [], + arrayedEquation: { + dimensions: ['regions'], + equation: '99', + elements: [], + hasExceptDefault: false, + }, + }); + expect(restored.equation.type).toBe('arrayed'); + const eq = restored.equation as ArrayedEquation; + expect(eq.dimensionNames).toEqual(['regions']); + expect(eq.elements.size).toBe(0); + expect(eq.defaultEquation).toBe('99'); + expect(eq.hasExceptDefault).toBe(false); }); }); @@ -1150,6 +1349,8 @@ describe('Model', () => { nonNegative: false, canBeModuleInput: false, isPublic: false, + activeInitial: undefined, + dataSource: undefined, data: undefined, errors: undefined, unitErrors: undefined, @@ -1166,6 +1367,8 @@ describe('Model', () => { nonNegative: false, canBeModuleInput: false, isPublic: false, + activeInitial: undefined, + dataSource: undefined, data: undefined, errors: undefined, unitErrors: undefined, @@ -1209,6 +1412,8 @@ describe('Project', () => { nonNegative: true, canBeModuleInput: false, isPublic: false, + activeInitial: undefined, + dataSource: undefined, data: undefined, errors: undefined, unitErrors: undefined, @@ -1225,6 +1430,8 @@ describe('Project', () => { nonNegative: true, canBeModuleInput: false, isPublic: false, + activeInitial: undefined, + dataSource: undefined, data: undefined, errors: undefined, unitErrors: undefined, @@ -1240,6 +1447,8 @@ describe('Project', () => { gf: undefined, canBeModuleInput: false, isPublic: false, + activeInitial: undefined, + dataSource: undefined, data: undefined, errors: undefined, unitErrors: undefined, @@ -1347,6 +1556,8 @@ describe('projectAttachData', () => { gf: undefined, canBeModuleInput: false, isPublic: false, + activeInitial: undefined, + dataSource: undefined, data: undefined, errors: undefined, unitErrors: undefined, @@ -1461,3 +1672,159 @@ describe('projectAttachData', () => { expect(v?.data).toBeUndefined(); }); }); + +// These tests pin the wire fields that an editor upsert (a full variable +// replace by UID) previously dropped, because datamodel.ts neither read them +// on the way in nor wrote them on the way out. The JSON key names are the +// source of truth in the Rust serializer src/simlin-engine/src/json.rs +// (all #[serde(rename_all = "camelCase")]): +// - Compat: activeInitial, nonNegative, canBeModuleInput, isPublic, dataSource +// - JsonDataSource: kind ("data"/"constants"/"lookups"/"subscript"), file, +// tabOrDelimiter, rowOrCol, cell +// - ArrayedEquation: dimensions, equation (the EXCEPT default), elements, +// hasExceptDefault +// - ElementEquation: subscript, equation, compat, graphicalFunction +// The engine-driven round-trip in datamodel-roundtrip-e2e.test.ts re-checks +// these names against the real serializer, so a wrong key here cannot pass +// silently. +describe('variable compat round-trip (silent data-loss regression)', () => { + it('round-trips activeInitial on a stock through compat', () => { + const json: JsonStock = { + name: 'level', + inflows: [], + outflows: [], + initialEquation: '0', + compat: { activeInitial: 'starting_level' }, + }; + const restored = stockFromJson(json); + expect(restored.activeInitial).toBe('starting_level'); + expect(stockToJson(restored).compat?.activeInitial).toBe('starting_level'); + }); + + it('round-trips activeInitial on a flow and an aux through compat', () => { + const flow = flowFromJson({ name: 'rate', equation: '1', compat: { activeInitial: '2' } }); + expect(flow.activeInitial).toBe('2'); + expect(flowToJson(flow).compat?.activeInitial).toBe('2'); + + const aux = auxFromJson({ name: 'a', equation: '1', compat: { activeInitial: '3' } }); + expect(aux.activeInitial).toBe('3'); + expect(auxToJson(aux).compat?.activeInitial).toBe('3'); + }); + + it('round-trips compat.dataSource (GET DIRECT DATA) on an aux', () => { + const json: JsonAuxiliary = { + name: 'imported', + compat: { + dataSource: { + kind: 'data', + file: 'data.xlsx', + tabOrDelimiter: 'Sheet1', + rowOrCol: 'A', + cell: 'B2', + }, + }, + }; + const restored = auxFromJson(json); + const ds = restored.dataSource as DataSource; + expect(ds.kind).toBe('data'); + expect(ds.file).toBe('data.xlsx'); + expect(ds.tabOrDelimiter).toBe('Sheet1'); + expect(ds.rowOrCol).toBe('A'); + expect(ds.cell).toBe('B2'); + + const out = auxToJson(restored).compat?.dataSource; + expect(out).toEqual(json.compat!.dataSource); + }); + + it('maps each dataSource kind and falls back to "data" for unknown', () => { + for (const kind of ['data', 'constants', 'lookups', 'subscript'] as const) { + const aux = auxFromJson({ + name: 'x', + compat: { dataSource: { kind, file: 'f', tabOrDelimiter: '', rowOrCol: '', cell: '' } }, + }); + expect(aux.dataSource?.kind).toBe(kind); + } + const unknown = auxFromJson({ + name: 'x', + compat: { dataSource: { kind: 'bogus', file: 'f', tabOrDelimiter: '', rowOrCol: '', cell: '' } }, + }); + expect(unknown.dataSource?.kind).toBe('data'); + }); + + it('round-trips the EXCEPT default equation and hasExceptDefault flag', () => { + const json: JsonAuxiliary = { + name: 'arr', + arrayedEquation: { + dimensions: ['dim'], + equation: '99', + hasExceptDefault: true, + elements: [{ subscript: 'a', equation: '1' }], + }, + }; + const restored = auxFromJson(json); + const eq = restored.equation as ArrayedEquation; + expect(eq.defaultEquation).toBe('99'); + expect(eq.hasExceptDefault).toBe(true); + + const out = auxToJson(restored).arrayedEquation!; + expect(out.equation).toBe('99'); + expect(out.hasExceptDefault).toBe(true); + }); + + it('infers hasExceptDefault from a present default when the flag is absent', () => { + // Mirrors the engine: legacy JSON without the flag infers true when a + // default equation is present. + const restored = auxFromJson({ + name: 'arr', + arrayedEquation: { dimensions: ['dim'], equation: '7', elements: [{ subscript: 'a', equation: '1' }] }, + }); + expect((restored.equation as ArrayedEquation).hasExceptDefault).toBe(true); + + // No default equation => no flag emitted on the way out. + const noDefault = auxFromJson({ + name: 'arr', + arrayedEquation: { dimensions: ['dim'], elements: [{ subscript: 'a', equation: '1' }] }, + }); + const eq = noDefault.equation as ArrayedEquation; + expect(eq.defaultEquation).toBeUndefined(); + expect(eq.hasExceptDefault).toBe(false); + const out = auxToJson(noDefault).arrayedEquation!; + expect(out.equation).toBeUndefined(); + expect(out.hasExceptDefault).toBeUndefined(); + }); + + it('round-trips per-element graphical functions and per-element activeInitial', () => { + const json: JsonAuxiliary = { + name: 'arr', + arrayedEquation: { + dimensions: ['dim'], + elements: [ + { + subscript: 'a', + equation: '1', + graphicalFunction: { yPoints: [0, 1, 2], xScale: { min: 0, max: 2 }, yScale: { min: 0, max: 2 } }, + compat: { activeInitial: 'a0' }, + }, + { subscript: 'b', equation: '2' }, + ], + }, + }; + const restored = auxFromJson(json); + const eq = restored.equation as ArrayedEquation; + const a = eq.elements.get('a') as ArrayedElement; + expect(a.equation).toBe('1'); + expect(a.graphicalFunction?.yPoints).toEqual([0, 1, 2]); + expect(a.activeInitial).toBe('a0'); + const b = eq.elements.get('b') as ArrayedElement; + expect(b.graphicalFunction).toBeUndefined(); + expect(b.activeInitial).toBeUndefined(); + + const out = auxToJson(restored).arrayedEquation!; + const outA = out.elements!.find((e) => e.subscript === 'a')!; + expect(outA.graphicalFunction?.yPoints).toEqual([0, 1, 2]); + expect(outA.compat?.activeInitial).toBe('a0'); + const outB = out.elements!.find((e) => e.subscript === 'b')!; + expect(outB.graphicalFunction).toBeUndefined(); + expect(outB.compat).toBeUndefined(); + }); +}); diff --git a/src/diagram/CLAUDE.md b/src/diagram/CLAUDE.md index a4b22838c..ecba3b89e 100644 --- a/src/diagram/CLAUDE.md +++ b/src/diagram/CLAUDE.md @@ -67,9 +67,9 @@ Most project/engine invariants now live in `ProjectController` (`project-control - **External view overrides a live gesture** (Canvas, issue #707): a `useEffect` keyed on `[props.view, liveViewport]` compares `props.view`'s offset/zoom VALUE against `refs.viewBaseline` (tracked while idle). While a gesture is live, `props.view` is expected to stay put (a gesture never commits mid-flight), so any value change seen with `liveViewport` still set is external (centerVariable, module navigation, undo) and drops the live viewport + cancels pending momentum/deferred commits -- the external view wins, with no stray commit. A self-commit clears `liveViewport` in the same React commit as its optimistic `props.view` update, so it is never misread as external; comparing by value (not snapshot identity) ignores a content-equal republished view. If a pointer-driven viewport gesture (drag-pan or pinch) is still physically in progress when this fires, it is also abandoned (interaction -> idle; `panBaseOffset`/`mouseDownPoint`/`pointerId`/`activePointers` cleared) -- otherwise a continued move would recreate `liveViewport` from the stale press-time anchor and the pointer-up could commit that abandoned gesture back over the external view. - **Optimistic view updates** (controller): `updateView()`/`queueViewUpdate()` call `applyOptimisticView()` (synchronous snapshot replace of the active model's view + `projectVersion += 0.001`) *before* awaiting the engine round-trip. Any new view-modifying handler must go through these controller methods to avoid flicker. - **updateProject preserves the live view** (controller): `ProjectController.updateProject()` rebuilds `project` from the engine's serialized JSON, then merges via `preserveLiveView()` so the active model's view comes from the live snapshot (the most recent optimistic view). Without this, a slow engine round-trip racing with a newer pan/move would snap the diagram back to the engine's older view. The live view is round-tripped through JSON to re-link element `var` refs and stock inflow/outflow UIDs against the incoming variables. -- **View-only updates never record undo history** (controller): the `queueViewUpdate` path calls `updateProject(serialized, { recordHistory: false, scheduleSave: false })` -- it refreshes `project` and bumps `projectVersion` but must not touch `projectHistory`/`projectOffset`: viewBox/zoom are serialized into the protobuf, so recording them would let a single momentum flick evict every real edit from the `MaxUndoSize` (5) buffer. Real edits go through `advanceProjectHistory` (project-history.ts), which discards the redo branch when editing after an undo. -- **Version vs generation bookkeeping** (controller): `projectVersion` carries the fractional cache-key scheme (`+0.01` on content edits/undo, `+0.001` on view-only updates; Canvas keys render caches off it). `projectGeneration` increments *only* when project content changes (history-recording edits and undo/redo), never on view-only updates or save-version writes. -- **Details panels are keyed by `projectGeneration`, not `projectVersion`** (Editor render): VariableDetails/ModuleDetails seed their Slate editors from props in **lazy `useState` initializers** (one run per mount, the function-component equivalent of the old constructor seed), so key-driven remounts -- not prop-sync effects -- are what refreshes them. The Editor builds the panel key from `controllerSnapshot.projectGeneration`; keying on `projectVersion` would remount an open panel on every pan frame and autosave completion, discarding in-progress edits. +- **Viewport-only updates never record undo history; discrete element edits do** (controller): there are two view-mutating paths and they record differently. The `queueViewUpdate` path (pan/zoom/momentum/resize, and the internal navigation/replay viewport restores) calls `updateProject(serialized, { recordHistory: false, scheduleSave: false })` -- it refreshes `project` and bumps `projectVersion` but must NEVER touch `projectHistory`/`projectOffset`: viewBox/zoom are serialized into the protobuf, so recording them would let a single momentum flick evict every real edit from the `MaxUndoSize` (5) buffer. The `updateView` path is for DISCRETE element/structure edits (create, delete, element/group move, label move, flow/link attach); each such edit passes `{ recordHistory: true }` so it becomes individually undoable, producing exactly one entry per edit. For the delete/flow-attach/create handlers that apply a content patch via `applyPatchOrReportError` BEFORE calling `updateView`, the snapshot recorded in that final `updateView` captures the engine state after BOTH the content patch and the view update -- one entry, no double-recording. Bare `updateView()` (no opts) stays non-recording. All recorded edits go through `advanceProjectHistory` (project-history.ts), which discards the redo branch when editing after an undo. +- **Version vs generation bookkeeping** (controller): `projectVersion` carries the fractional cache-key scheme (`+0.01` on content edits/undo, `+0.001` on view-only updates; Canvas keys render caches off it). `projectGeneration` increments on every history-recording edit and on undo/redo -- that is, both model-content edits (equation/table/module/sim-specs/rename via `applyPatch`/`applyPatchAndRefresh`) AND discrete layout edits (element/label move, flow/link attach, create, delete) that call `updateView` with `recordHistory: true`. It never increments on the viewport-only stream (`queueViewUpdate`: pan/zoom/momentum/resize) or on save-version writes. +- **Details panels are keyed by `projectGeneration`, not `projectVersion`** (Editor render): VariableDetails/ModuleDetails seed their Slate editors from props in **lazy `useState` initializers** (one run per mount, the function-component equivalent of the old constructor seed), so key-driven remounts -- not prop-sync effects -- are what refreshes them. The Editor builds the panel key from `controllerSnapshot.projectGeneration`; keying on `projectVersion` would remount an open panel on every pan frame and autosave completion, discarding in-progress edits. Discrete layout edits (element/label move, flow/link attach) now bump `projectGeneration` and so remount an open panel; this is safe because a canvas-driven edit first blurs the side-panel editor, which commits its in-progress text (the same blur-commit the click-away flows already rely on) before the move records. - **Save queue releases `inSave` in a `finally` block** (controller): a thrown save (e.g. host-side network failure) must not leave `inSave === true`, otherwise every subsequent edit silently queues forever. A save requested while one is in flight queues exactly one flush; the queued retry uses `version ?? currVersion` so a save that errored before the server returned a new version still attempts to flush the next edit. - **Engine lifecycle / StrictMode safety** (controller + Editor): the controller's `disposed` flag latches once; every async continuation short-circuits on it and releases any engine it opened (collapsing the old `unmounted`/`openInitialProjectTimer`/`undoRedoTimer` machinery). The Editor constructs a controller once in its `refs`-init guard and its mount effect rebuilds one whenever a prior cleanup disposed and cleared it, so a React 18 StrictMode mount -> unmount -> mount cycle builds a *fresh* controller on the second mount. - **Module navigation stack** (controller): the controller owns `modelName` and an immutable `modelStack` of `ModuleStackEntry` (each storing the child model name, module ident, and the parent's selection/viewBox/zoom). All navigation goes through `pushModule`/`popModule`/`navigateToLevel` pure functions; the controller's navigation methods restore the parent viewport internally via `queueViewUpdate` (its `modelName` updates synchronously first, so `getView()` resolves to the restored model with no deferral) and the model-scoped error cache via `refreshCachedErrors`. They return a `NavigationOutcome` with the selection the Editor should adopt. The undo-driven navigation reset (restored project lacks the viewed model -> reset to 'main') bumps `navResetSeq`; a post-commit `useEffect` in the Editor clears selection/details/tool exactly once per bump (an ordinary undo preserves them). diff --git a/src/diagram/Editor.tsx b/src/diagram/Editor.tsx index 1e3e12806..1313ca961 100644 --- a/src/diagram/Editor.tsx +++ b/src/diagram/Editor.tsx @@ -36,6 +36,7 @@ import { flowToJson, auxToJson, moduleToJson, + arrayedEquationToJson, type ModuleReference, } from '@simlin/core/datamodel'; import { defined, exists, setsEqual } from '@simlin/core/common'; @@ -62,6 +63,7 @@ import { applyGroupMovement } from './group-movement'; import { detectUndoRedo, isEditableElement } from './keyboard-shortcuts'; import { isStdlibModel } from './module-navigation'; import { countModelInstances } from './module-details-utils'; +import { buildModuleReferencePayload } from './module-wiring'; import { BreadcrumbBar } from './BreadcrumbBar'; import { ProjectController, type ProjectSnapshot, type EngineApi } from './project-controller'; @@ -644,8 +646,11 @@ export const Editor = React.memo(function Editor(props: EditorProps): React.Reac r.controller?.scheduleSimRun(); }; - const updateView = async (view: StockFlowView): Promise => { - await r.controller?.updateView(view); + // Discrete element/structure edits pass { recordHistory: true } so each one + // becomes individually undoable; the per-frame viewport stream goes through + // queueViewUpdate (never records). See the controller's updateView doc. + const updateView = async (view: StockFlowView, opts?: { recordHistory?: boolean }): Promise => { + await r.controller?.updateView(view, opts); }; const queueViewUpdate = async (view: StockFlowView): Promise => { @@ -838,7 +843,7 @@ export const Editor = React.memo(function Editor(props: EditorProps): React.Reac await applyPatchOrReportError(patch, 'delete'); } - await updateView({ ...view, elements, nextUid }); + await updateView({ ...view, elements, nextUid }, { recordHistory: true }); scheduleSimRun(); }, []); @@ -853,7 +858,7 @@ export const Editor = React.memo(function Editor(props: EditorProps): React.Reac return { ...element, labelSide: side }; }); - await updateView({ ...view, elements }); + await updateView({ ...view, elements }, { recordHistory: true }); }, [], ); @@ -906,7 +911,7 @@ export const Editor = React.memo(function Editor(props: EditorProps): React.Reac } } - await updateView({ ...view, nextUid: result.nextUid, elements: [...result.elements] }); + await updateView({ ...view, nextUid: result.nextUid, elements: [...result.elements] }, { recordHistory: true }); setState({ selection, flowStillBeingCreated: inCreation, @@ -953,7 +958,7 @@ export const Editor = React.memo(function Editor(props: EditorProps): React.Reac } view = { ...view, nextUid, elements }; - await updateView(view); + await updateView(view, { recordHistory: true }); setState({ selection }); }, []); @@ -1029,7 +1034,7 @@ export const Editor = React.memo(function Editor(props: EditorProps): React.Reac // even when the upsert errored). await applyPatchOrReportError(patch, 'variable creation'); - await updateView({ ...view, nextUid, elements }); + await updateView({ ...view, nextUid, elements }, { recordHistory: true }); setState({ selection: new Set(), }); @@ -1049,7 +1054,7 @@ export const Editor = React.memo(function Editor(props: EditorProps): React.Reac }); const elements = view.elements.map((el) => updatedElements.get(el.uid) ?? el); - await updateView({ ...view, elements }); + await updateView({ ...view, elements }, { recordHistory: true }); }, [], ); @@ -1553,15 +1558,10 @@ export const Editor = React.memo(function Editor(props: EditorProps): React.Reac }, }; } else if (eq.type === 'arrayed') { - return { - arrayedEquation: { - dimensions: [...eq.dimensionNames], - elements: [...eq.elements.entries()].map(([subscript, eqStr]) => ({ - subscript, - equation: eqStr, - })), - }, - }; + // Use the shared serializer so per-element graphical functions, per-element + // ACTIVE INITIAL equations, and the EXCEPT default round-trip through the + // upsert (a hand-rolled mapping here previously dropped them). + return { arrayedEquation: arrayedEquationToJson(eq) }; } return { equation: '' }; }; @@ -1620,14 +1620,15 @@ export const Editor = React.memo(function Editor(props: EditorProps): React.Reac }, }; } else if (variable.type === 'module') { - // Modules have no equations or graphical functions -- only units and docs + // Modules have no equations or graphical functions -- only units and docs. + // Use moduleToJson to preserve all fields (including compat flags + // canBeModuleInput, isPublic, dataSource), then override edited fields. + const base = moduleToJson(variable); op = { type: 'upsertModule', payload: { module: { - name: variable.ident, - modelName: variable.modelName, - references: variable.references.map((ref) => ({ src: ref.src, dst: ref.dst })), + ...base, units: newUnits ?? variable.units ?? undefined, documentation: newDocs ?? variable.documentation ?? undefined, }, @@ -1732,15 +1733,13 @@ export const Editor = React.memo(function Editor(props: EditorProps): React.Reac const variable = model.variables.get(ident); if (!variable || variable.type !== 'module') return; + // Preserve all fields (including compat) via moduleToJson; override the model ref. const op: JsonModelOperation = { type: 'upsertModule', payload: { module: { - name: variable.ident, + ...moduleToJson(variable), modelName: newModelName, - references: variable.references.map((ref) => ({ src: ref.src, dst: ref.dst })), - units: variable.units || undefined, - documentation: variable.documentation || undefined, }, }, }; @@ -1762,13 +1761,12 @@ export const Editor = React.memo(function Editor(props: EditorProps): React.Reac const variable = model.variables.get(ident); if (!variable || variable.type !== 'module') return; + // Preserve all fields (including compat) via moduleToJson; override units/docs. const op: JsonModelOperation = { type: 'upsertModule', payload: { module: { - name: variable.ident, - modelName: variable.modelName, - references: variable.references.map((ref) => ({ src: ref.src, dst: ref.dst })), + ...moduleToJson(variable), units: newUnits ?? variable.units ?? undefined, documentation: newDocs ?? variable.documentation ?? undefined, }, @@ -1794,15 +1792,13 @@ export const Editor = React.memo(function Editor(props: EditorProps): React.Reac const variable = model.variables.get(ident); if (!variable || variable.type !== 'module') return; + // Preserve all fields (including compat) via moduleToJson; override references. const op: JsonModelOperation = { type: 'upsertModule', payload: { module: { - name: variable.ident, - modelName: variable.modelName, + ...moduleToJson(variable), references: newReferences.map((ref) => ({ src: ref.src, dst: ref.dst })), - units: variable.units || undefined, - documentation: variable.documentation || undefined, }, }, }; @@ -1830,26 +1826,12 @@ export const Editor = React.memo(function Editor(props: EditorProps): React.Reac newModelName = getUniqueDuplicateName(moduleIdent, project); } - // Look up existing module to preserve metadata through the model reference change + // Look up existing module to preserve metadata (including compat) through the + // model reference change; the shared helper carries every field forward + // (incl. compat) and keeps this in lockstep with the duplicate-model path. const model = getModel(); const existingModule = model?.variables.get(moduleIdent); - const modulePayload: { - name: string; - modelName: string; - references?: { src: string; dst: string }[]; - units?: string; - documentation?: string; - } = { - name: moduleIdent, - modelName: newModelName, - }; - if (existingModule && existingModule.type === 'module') { - if (existingModule.references.length > 0) { - modulePayload.references = existingModule.references.map((ref) => ({ src: ref.src, dst: ref.dst })); - } - if (existingModule.units) modulePayload.units = existingModule.units; - if (existingModule.documentation) modulePayload.documentation = existingModule.documentation; - } + const modulePayload = buildModuleReferencePayload(existingModule, moduleIdent, newModelName); const patch: JsonProjectPatch = { projectOps: [{ type: 'addModel', payload: { name: newModelName } }], @@ -1908,26 +1890,13 @@ export const Editor = React.memo(function Editor(props: EditorProps): React.Reac }); } - // Preserve existing module metadata through the model reference change + // Preserve ALL existing module fields (incl. compat: canBeModuleInput / + // isPublic / dataSource) through the model reference change. The hand-built + // payload here previously dropped compat -- the same full-replace data-loss + // trap fixed elsewhere; use the shared helper so it matches its sibling. const currentModel = getModel(); const existingModule = currentModel?.variables.get(moduleIdent); - const dupModulePayload: { - name: string; - modelName: string; - references?: { src: string; dst: string }[]; - units?: string; - documentation?: string; - } = { - name: moduleIdent, - modelName: newModelName, - }; - if (existingModule && existingModule.type === 'module') { - if (existingModule.references.length > 0) { - dupModulePayload.references = existingModule.references.map((ref) => ({ src: ref.src, dst: ref.dst })); - } - if (existingModule.units) dupModulePayload.units = existingModule.units; - if (existingModule.documentation) dupModulePayload.documentation = existingModule.documentation; - } + const dupModulePayload = buildModuleReferencePayload(existingModule, moduleIdent, newModelName); // Combined patch: create model, copy contents, update module reference. // Engine processes projectOps before model ops (patch.rs). diff --git a/src/diagram/module-wiring.ts b/src/diagram/module-wiring.ts index e99f3b4f4..649837933 100644 --- a/src/diagram/module-wiring.ts +++ b/src/diagram/module-wiring.ts @@ -4,7 +4,31 @@ // pattern: Functional Core -- pure functions for immutable reference array manipulation -import type { ModuleReference } from '@simlin/core/datamodel'; +import { type ModuleReference, type Variable, moduleToJson } from '@simlin/core/datamodel'; +import type { JsonModule } from '@simlin/engine'; + +/** + * Build the upsertModule payload that re-points an existing module variable at + * `newModelName` while preserving every other field. upsertModule is a + * full-replace by UID, so hand-listing fields silently drops anything omitted -- + * notably compat (canBeModuleInput / isPublic / dataSource), the same + * silent-data-loss trap fixed elsewhere in this change. Basing the payload on + * moduleToJson(existing) and overriding only name/modelName carries every field + * forward. Falls back to a bare {name, modelName} when there is no existing + * module (or the variable at that ident is not a module). Shared by the + * create-model-for-module and duplicate-model-for-module handlers so the two + * cannot drift apart again. + */ +export function buildModuleReferencePayload( + existing: Variable | undefined, + moduleIdent: string, + newModelName: string, +): JsonModule { + if (existing && existing.type === 'module') { + return { ...moduleToJson(existing), name: moduleIdent, modelName: newModelName }; + } + return { name: moduleIdent, modelName: newModelName }; +} // A module reference `dst` is stored in the canonical module-qualified form // `{moduleIdent}·{port}`: the engine's `build_module_inputs` strips that prefix diff --git a/src/diagram/project-controller.ts b/src/diagram/project-controller.ts index 960406d00..7dd574580 100644 --- a/src/diagram/project-controller.ts +++ b/src/diagram/project-controller.ts @@ -667,11 +667,25 @@ export class ProjectController { } /** - * Optimistic view update: reflect the new view in the snapshot immediately - * (so the UI never flashes stale positions), then round-trip through the - * engine. View edits never record undo history. + * Optimistic view update for a DISCRETE element/structure edit (create, + * delete, element/group move, label move, flow/link attach): reflect the new + * view in the snapshot immediately (so the UI never flashes stale positions), + * then round-trip through the engine. + * + * `recordHistory` controls whether this edit advances the undo buffer; it + * defaults to false so the bare call stays a non-recording view refresh. Each + * discrete user edit passes `recordHistory: true` so it becomes individually + * undoable. For the handlers that apply a content patch (via + * applyPatchOrReportError) BEFORE calling this, the snapshot serialized here + * captures the engine state AFTER both the content patch and the view update, + * so a single recorded entry covers the whole edit -- no double-recording. + * + * The per-frame viewport stream (pan/zoom/momentum/resize) does NOT come + * through here -- it uses queueViewUpdate, which never records, so a momentum + * flick cannot evict real edits from the small undo buffer. */ - async updateView(view: StockFlowView): Promise { + async updateView(view: StockFlowView, opts: { recordHistory?: boolean } = {}): Promise { + const { recordHistory = false } = opts; this.applyOptimisticView(view); const engine = this.engine; @@ -687,7 +701,7 @@ export class ProjectController { this.reportError(err.message ?? 'Unknown error during view update'); return; } - await this.updateProject(await engine.serializeProtobuf(), { scheduleSave: true, recordHistory: false }); + await this.updateProject(await engine.serializeProtobuf(), { scheduleSave: true, recordHistory }); } /** diff --git a/src/diagram/tests/flow-attach.test.ts b/src/diagram/tests/flow-attach.test.ts index d2efdb16a..fd48f2f17 100644 --- a/src/diagram/tests/flow-attach.test.ts +++ b/src/diagram/tests/flow-attach.test.ts @@ -109,6 +109,8 @@ function makeStockVar(ident: string, inflows: string[] = [], outflows: string[] nonNegative: false, canBeModuleInput: false, isPublic: false, + activeInitial: undefined, + dataSource: undefined, data: undefined, errors: undefined, unitErrors: undefined, diff --git a/src/diagram/tests/module-details-utils.test.ts b/src/diagram/tests/module-details-utils.test.ts index c39231e69..6a66dddca 100644 --- a/src/diagram/tests/module-details-utils.test.ts +++ b/src/diagram/tests/module-details-utils.test.ts @@ -65,6 +65,9 @@ function makeModule(ident: string, modelName: string): Module { documentation: '', units: '', references: [], + canBeModuleInput: false, + isPublic: false, + dataSource: undefined, data: undefined, errors: undefined, unitErrors: undefined, diff --git a/src/diagram/tests/module-details.test.tsx b/src/diagram/tests/module-details.test.tsx index 393779f8d..17799163a 100644 --- a/src/diagram/tests/module-details.test.tsx +++ b/src/diagram/tests/module-details.test.tsx @@ -20,6 +20,8 @@ function makeAux(ident: string, overrides?: Partial): Aux { gf: undefined, canBeModuleInput: false, isPublic: false, + activeInitial: undefined, + dataSource: undefined, data: undefined, errors: undefined, unitErrors: undefined, @@ -40,6 +42,8 @@ function makeStock(ident: string, overrides?: Partial): Stock { nonNegative: false, canBeModuleInput: false, isPublic: false, + activeInitial: undefined, + dataSource: undefined, data: undefined, errors: undefined, unitErrors: undefined, @@ -56,6 +60,9 @@ function makeModule(ident: string, modelName: string, overrides?: Partial = {}): Module { + return { + type: 'module', + ident: 'hares_inst', + modelName: 'hares', + documentation: 'doc', + units: 'widgets', + references: [{ src: 'predators', dst: 'hares_inst·input_food' }], + canBeModuleInput: true, + isPublic: true, + dataSource: undefined, + data: undefined, + errors: undefined, + unitErrors: undefined, + uid: 7, + ...overrides, + }; +} + // -- Tests -- describe('qualifyDst', () => { @@ -254,3 +274,57 @@ describe('getAvailableSrcVariables', () => { expect(result).toEqual([]); }); }); + +describe('buildModuleReferencePayload', () => { + it('preserves compat (canBeModuleInput/isPublic/dataSource) while re-pointing the model', () => { + // upsertModule is a full-replace by UID, so re-pointing a module at a new + // model must carry every field forward -- compat is the data-loss trap. + const existing = makeModule({ + canBeModuleInput: true, + isPublic: true, + dataSource: { kind: 'data', file: 'd.csv', tabOrDelimiter: ',', rowOrCol: 'A', cell: 'B2' }, + }); + const payload = buildModuleReferencePayload(existing, 'hares_inst', 'hares_copy'); + expect(payload.name).toBe('hares_inst'); + expect(payload.modelName).toBe('hares_copy'); + expect(payload.compat?.canBeModuleInput).toBe(true); + expect(payload.compat?.isPublic).toBe(true); + expect(payload.compat?.dataSource).toEqual({ + kind: 'data', + file: 'd.csv', + tabOrDelimiter: ',', + rowOrCol: 'A', + cell: 'B2', + }); + // existing references/units/documentation also carry forward + expect(payload.references).toEqual([{ src: 'predators', dst: 'hares_inst·input_food' }]); + expect(payload.units).toBe('widgets'); + expect(payload.documentation).toBe('doc'); + }); + + it('returns a bare {name, modelName} when there is no existing module', () => { + const payload = buildModuleReferencePayload(undefined, 'new_inst', 'new_model'); + expect(payload).toEqual({ name: 'new_inst', modelName: 'new_model' }); + }); + + it('returns a bare payload when the existing variable is not a module', () => { + const aux: Aux = { + type: 'aux', + ident: 'not_a_module', + equation: { type: 'scalar', equation: '1' }, + documentation: '', + units: '', + gf: undefined, + canBeModuleInput: false, + isPublic: false, + activeInitial: undefined, + dataSource: undefined, + data: undefined, + errors: undefined, + unitErrors: undefined, + uid: 3, + }; + const payload = buildModuleReferencePayload(aux, 'x', 'y'); + expect(payload).toEqual({ name: 'x', modelName: 'y' }); + }); +}); diff --git a/src/diagram/tests/project-controller.test.ts b/src/diagram/tests/project-controller.test.ts index 87c3f2dd0..587b905d2 100644 --- a/src/diagram/tests/project-controller.test.ts +++ b/src/diagram/tests/project-controller.test.ts @@ -204,6 +204,78 @@ describe('ProjectController optimistic view updates', () => { await controller.dispose(); }); + it('updateView records undo history only when recordHistory is set', async () => { + // Discrete element/structure edits (create/delete/move/flow-attach/etc.) + // funnel their final engine state through updateView and must each produce + // exactly one undo entry. A plain updateView (the legacy default) and the + // viewport-only queueViewUpdate path must record nothing. + const engine = makeFakeEngine(); + const { config } = makeControllerConfig({ engine, format: 'protobuf', initialData: snap(1) }); + const controller = new ProjectController(config); + await controller.openInitialProject(); + + const view = controller.getView() as StockFlowView; + + // A plain updateView (no opts) refreshes the project but records nothing. + const genBefore = controller.getSnapshot().projectGeneration; + await controller.updateView({ ...view, zoom: 2 }); + expect(controller.canUndo()).toBe(false); + expect(controller.getSnapshot().projectGeneration).toBe(genBefore); + + // queueViewUpdate (pan/zoom/momentum) likewise records nothing. + await controller.queueViewUpdate({ ...view, zoom: 3 }); + expect(controller.canUndo()).toBe(false); + expect(controller.getSnapshot().projectGeneration).toBe(genBefore); + + // A discrete edit (recordHistory: true) advances history exactly once. + await controller.updateView({ ...view, zoom: 4 }, { recordHistory: true }); + expect(controller.canUndo()).toBe(true); + expect(controller.getSnapshot().projectGeneration).toBe(genBefore + 1); + + // undo then redo round-trips the cursor. + controller.undoRedo('undo'); + await flushTimers(); + expect(controller.canRedo()).toBe(true); + controller.undoRedo('redo'); + await flushTimers(); + expect(controller.canRedo()).toBe(false); + await controller.dispose(); + }); + + it('undo after a recordHistory updateView reopens the engine from the pre-edit snapshot', async () => { + // Prove restoration, not just the cursor move: the undo reopen must pull the + // serialized snapshot captured BEFORE the edit back into the engine. + const openedWith: Uint8Array[] = []; + let counter = 100; + const engine = makeFakeEngine({ protobuf: () => new Uint8Array([counter++]) }); + const config = { + initialProjectVersion: 1, + input: { format: 'protobuf' as const, data: new Uint8Array([1]) }, + openProtobuf: async (data: Uint8Array) => { + openedWith.push(data); + return engine; + }, + openJson: async () => engine, + save: async () => 1, + onError: () => {}, + }; + const controller = new ProjectController(config); + await controller.openInitialProject(); + // History head is the post-open snapshot ([100]). + const view = controller.getView() as StockFlowView; + await controller.updateView({ ...view, zoom: 9 }, { recordHistory: true }); + // The edit recorded a fresh head, so there is a distinct pre-edit snapshot + // to restore (without this the undo would merely clamp to the only entry). + expect(controller.canUndo()).toBe(true); + + controller.undoRedo('undo'); + await flushTimers(); + + // The reopen restored the pre-edit snapshot ([100]) into the engine. + expect(openedWith[openedWith.length - 1]).toEqual(new Uint8Array([100])); + await controller.dispose(); + }); + it('stashes the queued view when no engine is installed yet and replays it on reopen', async () => { // Drive queueViewUpdate before any engine exists (controller just // constructed), then confirm the next engine pulls the queued view. diff --git a/src/diagram/tests/variable-details-display.test.ts b/src/diagram/tests/variable-details-display.test.ts index 551462140..f0f5c4ac9 100644 --- a/src/diagram/tests/variable-details-display.test.ts +++ b/src/diagram/tests/variable-details-display.test.ts @@ -17,6 +17,8 @@ function aux(overrides: Partial = {}): Variable { gf: undefined, canBeModuleInput: false, isPublic: false, + activeInitial: undefined, + dataSource: undefined, data: undefined, errors: undefined, unitErrors: undefined, diff --git a/src/engine/src/json-types.ts b/src/engine/src/json-types.ts index 9eabcef10..240e3e536 100644 --- a/src/engine/src/json-types.ts +++ b/src/engine/src/json-types.ts @@ -30,6 +30,21 @@ export interface JsonGraphicalFunction { yScale?: JsonGraphicalFunctionScale; } +/** + * External-data reference (Vensim GET DIRECT DATA / CONSTANTS / LOOKUPS / + * SUBSCRIPT). Field names and the `kind` string values match the Rust + * `json::JsonDataSource` serializer in src/simlin-engine/src/json.rs + * (`#[serde(rename_all = "camelCase")]`; kind is one of + * `data`/`constants`/`lookups`/`subscript`). + */ +export interface JsonDataSource { + kind: string; + file: string; + tabOrDelimiter: string; + rowOrCol: string; + cell: string; +} + /** * Vensim compatibility options for a variable. */ @@ -38,6 +53,7 @@ export interface JsonCompat { nonNegative?: boolean; canBeModuleInput?: boolean; isPublic?: boolean; + dataSource?: JsonDataSource; } /** @@ -58,6 +74,12 @@ export interface JsonArrayedEquation { equation?: string; compat?: JsonCompat; elements?: JsonElementEquation[]; + /** + * Whether `equation` is an EXCEPT default applied to elements not covered + * by `elements` (Vensim EXCEPT). Matches the Rust `hasExceptDefault` field; + * only meaningful when a default `equation` is present. + */ + hasExceptDefault?: boolean; } /** @@ -85,6 +107,10 @@ export interface JsonStock { compat?: JsonCompat; /** @deprecated Legacy field; read from compat.nonNegative in new format. */ nonNegative?: boolean; + /** @deprecated Legacy field; read from compat.canBeModuleInput in new format. */ + canBeModuleInput?: boolean; + /** @deprecated Legacy field; read from compat.isPublic in new format. */ + isPublic?: boolean; } /** @@ -101,6 +127,10 @@ export interface JsonFlow { compat?: JsonCompat; /** @deprecated Legacy field; read from compat.nonNegative in new format. */ nonNegative?: boolean; + /** @deprecated Legacy field; read from compat.canBeModuleInput in new format. */ + canBeModuleInput?: boolean; + /** @deprecated Legacy field; read from compat.isPublic in new format. */ + isPublic?: boolean; } /** @@ -115,6 +145,10 @@ export interface JsonAuxiliary { documentation?: string; arrayedEquation?: JsonArrayedEquation; compat?: JsonCompat; + /** @deprecated Legacy field; read from compat.canBeModuleInput in new format. */ + canBeModuleInput?: boolean; + /** @deprecated Legacy field; read from compat.isPublic in new format. */ + isPublic?: boolean; } /** @@ -128,6 +162,10 @@ export interface JsonModule { documentation?: string; references?: JsonModuleReference[]; compat?: JsonCompat; + /** @deprecated Legacy field; read from compat.canBeModuleInput in new format. */ + canBeModuleInput?: boolean; + /** @deprecated Legacy field; read from compat.isPublic in new format. */ + isPublic?: boolean; } /**