From efccceff736ea002c721bfccbf9770d2a9eee0f6 Mon Sep 17 00:00:00 2001 From: PraharshNagpure Date: Wed, 2 Sep 2026 12:13:23 +0530 Subject: [PATCH 1/2] =?UTF-8?q?feat:=20v0.17.0=20=E2=80=94=20the=20QA=20en?= =?UTF-8?q?gine?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Validation becomes a rule engine an engineer can work through, instead of two lists split by an implicit flag. Severity is now a real thing The old model encoded it as the ABSENCE of a field — `severity?: 'suggestion'`, where undefined meant error — so there was no middle and no way to say how much a finding mattered. Rules now declare critical / warning / information and a discipline (tagging, topology, process, instrumentation, data), and the report groups by severity first because that is how an engineer triages: what blocks issue, then what to look at. Accepting a finding, with a reason Not every finding is a mistake. Accepting one records why, and keeps it visible in its own section rather than hiding it — a hidden acceptance rots, and the reason someone accepted it is exactly what the next reviewer needs. Because a finding is keyed by rule + ENGINEERING identity and never by node id, the acceptance survives deleting and redrawing the symbol. Fixes are data, not callbacks A rule returns a FixSpec, so a repair can be named, previewed with its blast radius, and replayed in a test before anything happens. applyFix reports whether it actually landed instead of failing silently, and describeFix gives one shared description so a preview and the action cannot drift. (Requested by another session building on this; it was the right call regardless, and converting the closures is what exposed the duplicate-key bug below.) One index per document Every rule reads a single prepared walk of the drawing. runChecks alone used to build four maps, and the advisor rebuilt a neighbour list per node per rule. Three bugs this found - Equipment tags were reported as ISA errors. invalid-letters ran the ISA-5.1 INSTRUMENT letter tables over equipment, so P-101 on a pump — a completely standard tag — was flagged. Found by running the new rules over the project's own bundled templates. The false positive existed in the old engine too. - The same finding was emitted twice when two symbols wore one tag, so accepting one would silently accept both. Findings are now one per rule and entity. - qaFor's cache never hit, because `doc.qa?.ignored ?? {}` allocated a fresh object every call, so all 21 rules re-ran on every render. Calibrated against real drawings, not intuition Running the rule set over the five bundled examples first reported criticals on three of them and 11-13 identical warnings. That teaches people to ignore the report, so: no-relief and no-fail-position are warnings rather than blockers by default (both are real, both are legitimately unstated on early drawings; a company standard promotes them in v0.18), and required-field-empty only fires on a record someone has STARTED — an object nobody has begun specifying is not yet an omission. All five bundled drawings now report zero criticals. Retires validate/checks.ts, validate/suggest.ts, validate/issues.ts and the two panels that read them; their coverage moved to the rule tests. locateCell — the primitive behind every jump in the app — moves to canvas/locate.ts, where it belongs. Removes the dead Finding type. Note on a shared working tree: this commit was made from a checkout in which another session was concurrently building the mid-drag docking work. Its own files are committed separately, but a few of its lines ride along here in files both changes touch — the .pid-dock-cut styles in app.css, dockNode returning the new edge id in store.ts, one README bullet, and its own [Unreleased] CHANGELOG section (left under its own heading, deliberately not folded into 0.17.0). Co-Authored-By: Claude Opus 5 (1M context) --- CHANGELOG.md | 76 ++ README.md | 7 +- .../2026-09-02-project-assistant-design.md | 656 ++++++++++++++++++ e2e/autoconnect.spec.ts | 102 ++- e2e/engineering.spec.ts | 3 +- e2e/happy-path.spec.ts | 9 +- e2e/qa.spec.ts | 80 +++ package.json | 2 +- src/WorkspaceRail.tsx | 10 +- src/app.css | 41 +- src/assist/fixes.ts | 86 ++- src/canvas/locate.ts | 71 ++ src/canvas/shapes.ts | 2 +- src/model/projectIndex.ts | 177 +++++ src/model/types.ts | 20 +- src/panels/AdvisorPanel.tsx | 60 -- src/panels/CommandPalette.tsx | 5 +- src/panels/Drawer.tsx | 4 +- src/panels/InspectorWhereUsed.tsx | 2 +- src/panels/IssuesPanel.tsx | 60 +- src/panels/StatusBar.tsx | 12 +- src/panels/ValidationPanel.tsx | 62 -- src/store/store.ts | 38 +- src/validate/checks.ts | 106 --- src/validate/engine.ts | 118 ++++ src/validate/issues.ts | 44 -- src/validate/rules.ts | 76 ++ src/validate/rules/data.ts | 112 +++ src/validate/rules/index.ts | 22 + src/validate/rules/instrumentation.ts | 188 +++++ src/validate/rules/process.ts | 69 ++ src/validate/rules/tagging.ts | 155 +++++ src/validate/rules/topology.ts | 150 ++++ src/validate/suggest.ts | 200 ------ src/workspaces/ChecksWorkspace.tsx | 194 +++--- src/workspaces/DataWorkspace.tsx | 6 +- tests/assist/fixes.test.ts | 11 +- tests/assist/typicals.test.ts | 6 +- tests/validate/checks.test.ts | 74 -- tests/validate/engine.test.ts | 132 ++++ tests/validate/issues.test.ts | 58 -- tests/validate/rules.test.ts | 227 ++++++ tests/validate/suggest.test.ts | 113 --- 43 files changed, 2736 insertions(+), 910 deletions(-) create mode 100644 docs/superpowers/specs/2026-09-02-project-assistant-design.md create mode 100644 e2e/qa.spec.ts create mode 100644 src/canvas/locate.ts create mode 100644 src/model/projectIndex.ts delete mode 100644 src/panels/AdvisorPanel.tsx delete mode 100644 src/panels/ValidationPanel.tsx delete mode 100644 src/validate/checks.ts create mode 100644 src/validate/engine.ts delete mode 100644 src/validate/issues.ts create mode 100644 src/validate/rules.ts create mode 100644 src/validate/rules/data.ts create mode 100644 src/validate/rules/index.ts create mode 100644 src/validate/rules/instrumentation.ts create mode 100644 src/validate/rules/process.ts create mode 100644 src/validate/rules/tagging.ts create mode 100644 src/validate/rules/topology.ts delete mode 100644 src/validate/suggest.ts delete mode 100644 tests/validate/checks.test.ts create mode 100644 tests/validate/engine.test.ts delete mode 100644 tests/validate/issues.test.ts create mode 100644 tests/validate/rules.test.ts delete mode 100644 tests/validate/suggest.test.ts diff --git a/CHANGELOG.md b/CHANGELOG.md index 967ba54..a7b0f47 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,82 @@ All notable changes to IPD Studio. Format follows [Keep a Changelog](https://keepachangelog.com/en/1.1.0/); versions follow [Semantic Versioning](https://semver.org/spec/v2.0.0.html). +## [Unreleased] + +### Changed + +- **Docking happens mid-drag, not on release.** The line is drawn the moment + the two connection points meet, with the mouse button still down — keep + dragging and the pipe stretches behind you. Before, you had to let go, + then pick the symbol back up to pull it into place. +- **Symbols stand off 24px from the point they dock onto**, in line with the + way that port faces, so a real length of pipe is visible. Landing the + points on top of each other read as nothing having happened: the two + symbols butted together and hid the line behind themselves. +- **A magnet only joins ports that face each other.** Two ports pointing the + same way would stand a symbol on the wrong side of the nozzle it just + connected to, with its inlet pointing away. + +### Added + +- **Shake to disconnect** — waggle the symbol while still dragging and the + line that drag just made is cut, with a red flash where it used to land. + The symbol stays in hand, and it won't snap back onto the point just + rejected, so you can take it somewhere else without letting go. + +## [0.17.0] — 2026-09-02 — the QA engine + +Third step of the engineering-platform plan +([docs/ENGINEERING-PLATFORM-PLAN.md](docs/ENGINEERING-PLATFORM-PLAN.md), §4.2). +Validation becomes a rule engine an engineer can actually work through. + +### Added + +- **21 rules with real severities** — critical, warning and information, grouped + by discipline (tagging, topology, process, instrumentation, data). Severity + used to be encoded as the *absence* of a field, which could not express the + middle. +- **Accept a finding, with a reason.** Not every finding is a mistake. Accepting + one records why, keeps it visible in its own section rather than hiding it, + and — because findings are keyed by rule + engineering identity, never by node + id — the acceptance survives deleting and redrawing the symbol. +- **Fixes are data, not callbacks.** A rule returns a `FixSpec`, so a repair can + be named, previewed with its blast radius, and replayed in a test before it is + applied. `applyFix` now reports whether it actually landed instead of failing + silently, and `describeFix` gives one shared description of what a fix would do. +- **One index per document.** Every rule reads a single prepared walk of the + drawing rather than rebuilding its own maps — `runChecks` alone used to build + four, and the advisor rebuilt a neighbour list per node per rule. +- **The Checks workspace** groups by severity, filters by discipline, states why + each rule matters, and keeps accepted findings visible with their reasons. + +### Fixed + +- **Equipment tags are no longer reported as ISA errors.** `invalid-letters` was + applying the ISA-5.1 *instrument* letter tables to equipment, so `P-101` on a + pump — a completely standard tag — was flagged as invalid. This was found by + running the new rule set over the project's own bundled templates, and the + false positive existed in the old engine too. +- **One finding per rule and entity.** Rules that walk nodes emitted the same + finding twice when two symbols wore one tag, which also meant accepting one of + them silently accepted both. + +### Changed + +- `no-relief` and `no-fail-position` are **warnings, not blockers**, by default. + Both are real concerns and both are legitimately unstated on plenty of + drawings — three of the five bundled samples trip `no-relief`. A company + standard promotes them (v0.18); crying wolf until then teaches people to + ignore the report. +- `required-field-empty` only fires on a record someone has **started**. Firing + on every tagged object of a pre-registry drawing buried the report under + 11–13 identical warnings. +- The drawer's Issues tab shows criticals and warnings; observations live in the + Checks workspace. The rail badge counts criticals only. +- Retired `validate/checks.ts`, `validate/suggest.ts`, `validate/issues.ts` and + the two panels that read them; their coverage moved to the rule tests. +- Removed the dead `Finding` type. + ## [0.16.0] — 2026-09-01 — magnetic docking ### Added diff --git a/README.md b/README.md index 63201cb..b654a23 100644 --- a/README.md +++ b/README.md @@ -106,9 +106,10 @@ P&ID tool is a $2,600+/year desktop install. IPD Studio is the missing thing: - **Works offline** — installable PWA; the whole editor runs with no internet - **Editor power** — Ctrl+F find-any-tag across sheets, align/distribute, live snap guides, print-all-sheets PDF, line sequence auto-numbering -- **Magnetic docking** — drag a symbol so its connection point touches - another symbol's and let go: it clicks into place already piped up. Pull - them apart and the line stretches to follow +- **Magnetic docking** — drag a symbol so its connection point meets another + symbol's and the pipe is drawn there and then, mid-drag: keep dragging and + it stretches to follow. Wrong point? Shake the symbol and the line is cut, + without ever letting go - **Obstacle-avoiding orthogonal routing** with draggable waypoints, ports with connection rules (a pneumatic signal won't connect to a pipe nozzle) - **Live validation**: duplicate tags, missing tags, illegal ISA letters, diff --git a/docs/superpowers/specs/2026-09-02-project-assistant-design.md b/docs/superpowers/specs/2026-09-02-project-assistant-design.md new file mode 100644 index 0000000..98d8a9e --- /dev/null +++ b/docs/superpowers/specs/2026-09-02-project-assistant-design.md @@ -0,0 +1,656 @@ +# The project assistant — design + +Date: 2026-09-02 +Status: approved, awaiting implementation plan +Target versions: v0.17.0 (foundations), v0.18.0 (assistant v1) +Written against v0.16.0 (schemaVersion 5, 20,273 LOC, HEAD `be431e9`) + +## Problem + +The user wants an in-app assistant that works like an engineer: it answers +questions about the drawing, predicts and finds errors, and can edit or delete +in the workspace — with every action gated by an explicit allow/deny prompt, and +with full awareness of what is currently selected. + +Two constraints come from the codebase's own prior decisions and are honoured +rather than overturned: + +- `ENGINEERING-PLATFORM-PLAN.md` §5.5 scopes the assistant to *grounded + retrieval over the entity index*, producing jump lists into real objects. +- §9 lists "a generic AI chatbot" under what not to build: *"An assistant that + cannot ground an answer in the model will invent tags."* + +This design satisfies the user's request **and** those constraints, by making +grounding a structural property rather than a prompt instruction. + +## The product decision + +> **Not an LLM in your P&ID. A query engine over the plant model, with an LLM on +> the semantic quarter that queries cannot reach.** + +Of twenty real questions an instrument engineer asks about a selected loop or +component (§6), **eleven are database queries, four are hybrids, one is domain +opinion, and four must be refused** because the document does not hold the data. + +The eleven are answered by a deterministic template layer with **no model call +at all** — exact, instant, free, unhallucinatable. The LLM earns its keep on the +remaining semantic work, and on four questions its most valuable behaviour is +declining. + +## Measured facts that shaped this + +Measured against `examples/sample-refinery-unit.pnid.json` (3 sheets, 32 nodes, +30 edges): + +| Measurement | Value | Consequence | +|---|---|---| +| Untagged-but-labelled nodes | **14 of 32 (44%)** | Nearly half the plant's identity lives in free text (`"TK-201 Crude Feed"`) that `deriveLoops` skips (`src/store/selectors.ts:18`) and `keyOfNode` cannot key (`src/model/registry.ts:60`). This is the LLM's strongest justification *and* the reason the assistant must often answer "that object is untagged". | +| Process edges carrying `arrow: 'flow'` | **4 of 20 (20%)** | Direction is *not knowable* for most lines. "What is downstream?" must be answered as adjacency with an explicit caveat, never guessed. | +| Whole document, distilled to engineering facts | **1,926 B ≈ 482 tokens** | Context size was never the constraint. Correctness is. A ~2 kB selection brief is affordable at any project size. | + +## Decisions taken + +| Decision | Choice | +|---|---| +| Brain | Hybrid. The LLM plans; it may only act through typed tools and may only assert facts a tool returned. | +| Tool surface | Two tiers. Intent-level tools composed from store actions — **never** a 1:1 wrapper (see below). | +| Read tools | Loop freely, unattended, no consent prompt. | +| Write tools | A frozen, reviewed plan. One bounded "re-plan once, then re-approve" escape hatch. | +| Unit of consent | The `FixSpec` — one engineering intention — never the store call, and never two `FixSpec`s behind one button. | +| Unit of undo | Identical to the unit of consent. One approved action = one `undo()`. | +| Destructive tools | None in v1. | +| Selection | The assistant never calls `setSelection`. It gets a separate non-undoable highlight channel. | +| Context | A ≤2 kB selection brief, preloaded and frozen into the message at send time. Depth beyond one hop comes from tools. | +| Transport | Behind an interface. BYO key in v1; hosted proxy deferred. | +| UI | A dock in the existing right column, mutually exclusive with the property panel. | + +### Why a 1:1 store wrapper is incorrect, not merely inelegant + +Every node/edge mutation routes through `patchSheet` (`src/store/store.ts:149`), +which only ever rewrites the **active** sheet. `setTag` resolves `activeSheet(s)` +and then bails silently: + +```ts +// src/store/store.ts:315-317 +const sheet = activeSheet(s) +const node = sheet.nodes.find((n) => n.id === id) +if (!node) return s +``` + +So "retag FT-201 on sheet-process" while Utilities is active is a **silent no-op +that throws nothing and returns nothing**. `applyFix` already hand-codes the +workaround (`src/assist/fixes.ts:27`). Exposing sixty such actions gives a model +sixty ways to report success on work it never did — the precise failure the +grounding rule exists to prevent. + +Second reason: `getSymbol` **throws** on an unknown id +(`src/symbols/registry.ts:16`) and `addNode` validates nothing +(`src/store/store.ts:188`). Unguarded call sites include `src/validate/checks.ts`, +`src/export/csv.ts` and `src/canvas/shapes.ts`. A hallucinated `symbolId` does +not draw the wrong thing — it white-screens the app on the next validation pass. +Whitelisting is crash prevention. + +## Architecture + +``` +selection ──► buildIndex(doc) memoised on doc identity, like issuesFor + │ + ├──► selectionBrief() ≤2 kB, deterministic, frozen at send + │ + ├──► TEMPLATE LAYER 11 of 20 question types, zero model calls + │ + └──► AGENT LOOP + read tools ───────── free, unattended, no consent + │ + proposed FixSpec[] ─ serializable values, never closures + │ + describeFix() ────── consent card + blast radius + ghost preview + │ + applyFix() ───────── { ok, changedIds }, one undo step + │ + grounding validator blocks unknown tags BEFORE render + │ + citation chips ───── [[ref|id]] → pid-cite highlight channel +``` + +## 1. Foundations — `buildIndex` and the rule interface + +> **Status note, 2026-09-02.** Most of this section was built in a parallel +> session while this spec was being written, and is present in the working tree +> uncommitted: `src/model/projectIndex.ts` (the index), `src/validate/rules.ts` +> (`Rule` / `RuleFinding`), `src/validate/engine.ts` (`runRules`, severity +> ordering, per-rule crash containment), `src/validate/rules/` (21 rules across +> five disciplines), and `doc.qa.ignored` for auditable ignores. What remains for +> v0.17.0 is the fix representation (§5) and the five bugs (§16). Verify against +> the tree before implementing anything here. + +Prerequisite work, shipping in v0.17.0. **Not** the full 24-rule QA engine from +plan §4.2 — only the two pieces that prevent the assistant being rewritten when +that engine lands: + +1. **`buildIndex(doc): ProjectIndex`** in `src/store/selectors.ts`, memoised on + document identity with the same single-entry cache as + `src/validate/issues.ts:30-39` and for the same stated reason. One walk + produces: nodes by id · edges by node · tags → nodes · loops · records by key · + neighbour lists · port kinds. This also fixes plan finding #7 (validation + running 3× per keystroke). +2. **Stable finding identity** — `ruleId + entityKey`, replacing the current + `checkId:targetId` strings (`src/validate/checks.ts:24`). The existing 16 + rules are ported unchanged; no new rules. Stable ids are what let the + assistant say *"this finding — the one you ignored last week"*, and what stops + it hard-coding a two-value severity where absence means error + (`src/model/types.ts:186`). + +### Reconciling the two definitions of "loop" + +The codebase currently holds two incompatible ones, and the disagreement between +them is where real drawing errors live: + +- `deriveLoops()` groups by first letter + loop number across all sheets and + **never reads an edge** (`src/store/selectors.ts:15-24`). +- The `no-final-element` rule asks whether a valve is reachable over signal lines + within 3 hops (`src/validate/suggest.ts:114`). + +`buildIndex` computes **both** and exposes the diff (`taggedNotWired`, +`wiredNotTagged`). Neither is deleted; the difference is a finding. + +## 2. The selection brief — `src/assist/context.ts` + +One pure function, `selectionBrief(index, activeSheetId, selection)`. This is the +only thing the model learns about the selection without a tool call. Hard-capped +at ~2 kB serialized; `budget.truncated: true` instructs the model to call a tool +rather than infer. + +For a single node the brief carries: id (the citation key) · ref · sheet · +kind · symbol · tag with `expandLetters()` and `validateLetters()` output · +label · config **and `configOptions`** · ports with connection state · the +engineering record read through `fieldValue()` (so the pre-v5 `node.datasheet` +fallback applies) with `filled/total` counts · neighbours · open findings. + +`configOptions` is load-bearing: handing the model the *closed set* of legal +values (`src/symbols/lib/valves-control.ts:125`) is what lets it propose +`fail: "fc"` and never `fail: "fail-shut"`. + +### Walk depths, and why each is that number + +| Walk | Depth | Reuses | Rationale | +|---|---|---|---| +| Process | Expand through pass-through hardware only; include the first non-pass-through node, flagged `terminal` | `passesThrough()` / `propagateFluid()`, `src/model/fluidFlow.ts:17,31` | The process question is always "what is this between?" This is already the boundary `setEdgeFluid` uses to spread a service (`src/store/store.ts:526`), so the brief and the app cannot disagree. Cap 40 nodes. | +| Signal | 3 hops over `signal.*` + `link.internal` | `signalReach()`, `src/validate/suggest.ts:45` | 3 is already the number `no-final-element` uses (`suggest.ts:114`). Matching it means the assistant and the Advisor can never contradict each other. It covers transmitter → I/P → valve. Past 3, a multi-drop bus swallows the sheet. | +| Tag family | Project-wide, zero hops | `deriveLoops()`, `src/store/selectors.ts:15` | A pure tag join; cheap, and what every deliverable already uses. | + +Everything past those bounds is a **query, not context** — the model calls +`walk_process(id, hops)` / `walk_signal(id, hops)` so the extra facts arrive as +tool results (assertable, citable) rather than free prompt text. + +### Per-shape rules + +- **1 node** — full focus, both neighbourhoods, loop block if tagged. +- **1 edge** — focus edge, both end nodes at full depth, the run, the line + record, plus `arrowsMarked / arrowsTotal` so the model knows when direction is + unknowable. +- **2–8 objects** — focus-lite each, one de-duplicated union neighbourhood, a + loop block per distinct `(family, number)`, plus `commonality` (same loop? same + run? contiguous?). "Why did you select these five together" is the real question. +- **>8 objects** — aggregate only: counts by kind, tag families, loops touched, + findings by rule, top 10 findings. A 200-object selection is a scope, not a subject. +- **Mixed** — first node is focus, rest shallow, `kind: "mixed"` so the model asks. +- **Empty** — the active sheet's aggregate, and the assistant must *say* "nothing + is selected; I'm answering about sheet Process." Never silently widen to the project. + +## 3. The template layer + +Answers the eleven model-free question types with no network call. Each template +renders a jump list, never prose about objects. + +**Matching is deterministic in v1** — keyword patterns plus selection shape, in +the style of `src/search/`'s existing matching. The LLM is not used to classify +intent, because a classifier call costs the same round trip the template layer +exists to avoid, and a misclassification would silently route a query question +into the generative path. On no match, the question goes to the agent loop. + +This layer is the thesis: half the catalogue is a query, and queries are exact, +instant, free and unhallucinatable. They buy the trust the LLM will spend. + +## 4. Read tools + +All pure over `buildIndex`, all returning ids, none prompting for consent. Every +result carries `rev` (§8) and a `total` alongside its rows — a result silently +capped at 50 is how a model concludes "there are no others". + +| Tool | Returns | +|---|---| +| `get_object(id)` | The focus shape from §2 | +| `get_loop(family, number)` | Both loop definitions, roles via `classifyMember` (`src/export/loopDiagram.ts:19`), the matching `TYPICALS` template and what is missing | +| `walk_process(id, hops)` | Run membership on the `propagateFluid` boundary | +| `walk_signal(id, hops)` | `signalReach` results | +| `list_findings(scope)` | `issuesFor(doc)` rows with stable ids and `hasFix` | +| `get_record(key)` | The record **plus the field catalog for its kind** (`src/model/fields.ts:150`) — which is what stops the model inventing a field key | + +`get_loop` uses `classifyMember` so "the final element" means the same thing to +the assistant as it does to the generated loop diagram. + +## 5. Write tools — the fix vocabulary + +`FixSpec` stays a **serializable discriminated union** +(`src/validate/suggest.ts:11`). The plan's `fix: { label, apply() }` closure idea +(§4.2) is explicitly rejected: a closure cannot be shown in a consent dialog as +data, logged, or replayed in a test. Rules return a `FixSpec` value; the model +*proposes* one; the user approves; `applyFix` dispatches. + +> **Conflict to resolve, 2026-09-02.** The in-flight QA engine adopted the +> closure form — `src/validate/rules.ts:26-29` defines +> `interface Fix { label: string; apply(): void }`. This must change before the +> rules multiply. Either form below satisfies the spec; the second requires no +> call-site rewrites: +> +> - `RuleFinding.fix?: { label: string; spec: FixSpec }`, or +> - `{ label, spec, apply() }` where `apply()` delegates to `applyFix(spec)`. +> +> Without one of them the assistant can only *run* a fix, never show the user +> what it would do first — which defeats the consent model the feature rests on. + +Two required changes to the existing dispatcher: + +- **`applyFix` must return `{ ok, changedIds, message }`.** Today it returns + `void` and silently no-ops on bad input (`src/assist/fixes.ts:31,36`). A tool + that fails silently cannot ground a follow-up claim. +- **Add `describeFix(fix, doc) → { title, blastRadius, affectedIds }**, so the + consent card and the assistant share one description. + +v1 ships **three** fixes, each chosen to prove one mechanism: + +| Fix | Rule | Proves | +|---|---|---| +| `assign-tag` | `missing-tag`, `src/validate/checks.ts:41` | Tag inheritance via `suggestLoop` (`src/isa/autonumber.ts:57`) — the model never authors a loop number | +| `set-fail-position` | `no-fail-position`, `src/validate/suggest.ts:136` | Writes **both** `setNodeConfig` and `setRecordField` — the symbol renders from `config`, the datasheet reads the record. Writing one creates exactly the drawing-vs-paperwork divergence this product exists to prevent | +| `delete-duplicate-line` | `duplicate-line`, `src/validate/suggest.ts:152` | Trivially safe, trivially undoable, currently the most obviously fixable suggestion with no fix attached | + +Designed and deferred to v2: `renumber-tag`, `add-relief`, `link-offpage`, +`adopt-line-service`, `set-line-number`, `swap-symbol`, `add-loop-member`. + +## 6. The question catalogue + +The behavioural spec. **M** = answerable from the document. **D** = needs domain +knowledge the LLM supplies. **U** = unanswerable; the assistant must say so. + +| # | Question | Class | +|---|---|---| +| 1 | Is this loop complete? | M (+D garnish) | +| 2 | What is FIC-201 controlling? | M, partial | +| 3 | What fails if PT-101 fails? | M + D, split | +| 4 | Why is this valve fail-closed? | **U** → redirect | +| 5 | What's my hazard here? | D, heavily caveated — partly U | +| 6 | Does this vessel have relief? | M | +| 7 | Is this PSV sized right? | **U** | +| 8 | What instruments are on this line? | M | +| 9 | Which loops are missing a controller? | M | +| 10 | Why did the Advisor say "FT-201 measures but nothing receives it"? | M | +| 11 | What's downstream of this valve? | M, mandatory caveat | +| 12 | Is this tag ISA-legal? | M | +| 13 | Should this be FIC or FC? | M + D | +| 14 | What signal type should drive this valve? | M | +| 15 | What's the calibrated range of this transmitter? | M, retrieval only | +| 16 | Which instruments have no datasheet? | M | +| 17 | What changed since revision 2? | **U** | +| 18 | Is this interlock complete / what trips this? | **U**, mostly | +| 19 | How much does this loop cost? | M | +| 20 | Does the HMI screen for this loop show everything it should? | M | + +Three that define the product's character: + +- **#4 is the acceptance test for the whole grounding design.** The document + records *that* a valve is FC (`config.fail`), never *why*. No rationale field + exists in `FIELD_CATALOG`. The assistant must say so, then offer the convention + as clearly-marked opinion. +- **#3 is the best grounding demo.** Reach is mechanical (`signalReach`); + behaviour on loss of signal is retrieval from `signal.fail` and each valve's + `config.fail`. When `fail` is `'none'` — the `no-fail-position` case — the + correct answer is *"unknown, and that is the finding."* +- **#15 must never infer a range from the service name.** That is precisely the + plausible-looking hallucination that destroys trust. Empty means empty, plus an + offer to fill it. + +**Refusal is a feature, but never a dead end.** Every "I can't" ships with what +is missing, which field would hold it, and a one-click fix to create it. + +## 7. Grounding enforcement — `src/assist/grounding.ts` + +A pure function `validate(answer, doc, toolResults) → Violation[]`. Two layers. + +**Layer 1 — structured citations.** The answer schema forbids bare identifiers. +Prose references objects only as `[[ref|id]]`. The renderer resolves the id in +the index and renders **the document's current ref, not the model's string**. +That inversion is the whole trick: the model cannot make the UI display +"FT-205" for a node named "FT-201", because the UI never renders model text. + +**Layer 2 — free-text tag scan.** Models write bare tags in prose anyway. The +scanner already exists: `TAG_RE` (`src/isa/tag.ts:23`), un-anchored to a global +word-boundary form, with hits checked against `liveKeys(doc.sheets)` +(`src/model/registry.ts:108`) — which already yields every tag *and* line number +worn by anything on any sheet. + +| Case | Action | +|---|---| +| Well-formed tag **absent from the document** | **Hard block + one regeneration**, violation fed back ("FT-205 does not exist; the flow instruments here are FE-201, FT-201"). Second failure → refuse and render the raw finding list | +| **Known** tag outside the brief | Warn, don't block. The allow-set is brief ids ∪ every id any tool returned this turn | +| Tag-shaped string inside a quoted user phrase or fenced block | Skip — echoing the user's own typo must not trigger a block | + +Also validated: line numbers via `keyOfEdge` · symbol ids via `SYMBOLS.has` +(never `getSymbol`, which throws) · field keys via `fieldKeysFor(kind)` · +proposed ISA letters via `validateLetters` · and **every numeric claim must carry +`{ value, source: { id, fieldKey } }`**, re-read and string-compared. Mismatch +blocks. + +**Honest limitation:** this cannot catch a false *relationship* between two real +objects ("FT-201 is downstream of FV-201" when it is upstream). Mitigation: every +relational claim carries the id of the tool call that produced it, and the +validator checks the claim's objects appear in that call's result. Not airtight, +but it converts "the model asserted" into "the model cited" — auditable and +testable. + +## 8. Consent + +Three tiers, decided by what `undo()` can restore. + +| Tier | Contents | Prompt | +|---|---|---| +| **R — read/navigate** | All read tools. Also selection, active sheet and viewport: `zundo` does not even record them (`partialize: (state) => ({ doc: state.doc })`, `src/store/store.ts:891`) | Never. A card for "jump to FT-101" trains the user to stop reading cards | +| **M — mutating, undoable** | The three v1 fixes | One card per `FixSpec` | +| **D — destructive** | `deleteIds`, `deleteSheet`, `purgeRecord`, `removeFluid` | **Not exposed in v1** | + +`loadIntoStore` is not a tool at any tier: it calls +`temporal.getState().clear()` and sets `cloudId: null` (`src/store/store.ts:650`), +destroying undo history and unlinking the cloud drawing. Nor is there any generic +`applyPatch(json)` escape hatch — a typed boundary with one generic tool is not a +boundary. + +**The card shows, in this order:** the model's prose (marked as the model +talking); a **structured diff rendered by the app from the typed tool arguments, +never from model text**; a ghost preview for spatial fixes; and for anything with +reach, a blast-radius line computed by walking the document. If prose and diff +disagree, the diff is what happens. + +**Options:** `Allow` · `Allow this kind for this session` · `Deny` · `Deny and +say why`. The last is the highest-value and cheapest — free text goes back as the +tool result, so a rejection becomes steering rather than a dead end. + +**Session grants live in a module-scoped `Set`, cleared on `loadIntoStore`.** +Never in `doc` (it is exported and emailed — a grant travelling inside a `.pnid` +would pre-authorise an assistant on a stranger's machine), never in localStorage +(per-origin, so a grant made while doodling would apply to a client's +confidential drawing), never in Firestore. + +### Staleness + +A module-level `docRev` counter, bumped by a store subscriber on doc identity +change (same pattern as `src/persist/autosave.ts:52`). Every read result carries +`rev`; every write carries `expectRev`, checked at commit time inside the +transaction. This closes the window where the user drags a symbol during the +seconds a consent dialog is open. + +## 9. Undo + +**One consented `FixSpec` = one undo step.** The primitive exists and needs no +new store code: `pauseHistory` / `resumeHistory` (`src/store/store.ts:904-909`). +The sequence matters — let the *first* mutation record normally, then pause, then +run the rest, then resume, exactly as the label drag does at +`src/canvas/Canvas.tsx:156`. Pausing before the first mutation records nothing, +and the user's next Ctrl+Z would undo *their own* previous edit. `try/finally` +around `resumeHistory()` is mandatory. + +This matches the codebase's stated principle — `dockNode`'s comment +(`src/store/store.ts:33`): the move and the line it creates are one undo step +*"because they are one gesture to the user."* + +**"Undo everything the assistant just did"** is a journal plus counted `undo()`, +never a snapshot restore. Record the `doc` reference before and after each commit +(identity is already the change signal everywhere: `reconciler.ts`, +`issues.ts:30`). Revert = call `undo()` until `doc === beforeRef`. If the current +`doc` no longer matches the recorded `after`, the user has edited since — +**don't offer bulk revert at all**; offer "show me what this changed". Honest +beats clever, and `limit: 200` (`src/store/store.ts:892`) means a big enough +burst is not fully undoable anyway. + +The transcript *is* the journal — same object, two readings: conversation going +down, audit log going up. No second panel. + +## 10. Highlighting and citations + +### Canvas → assistant + +The panel subscribes to `s.selection` the way `syncSelection` already does +(`src/canvas/interactions.ts:537`), recomputes the brief, and renders a **context +chip strip** above the composer: `[FE-201 ×] [FIC-201 ×] [+ include loop F-201]`. +That strip *is* the feature — the user sees what the assistant is about to be +told, and can trim it. The brief is recomputed live but **frozen into the message +at send time** and shown inline, so scrollback never lies about what an old +answer was grounded in. + +### Assistant → canvas + +A second, **non-undoable** highlighter channel `pid-cite` alongside +`pid-selection` (`src/canvas/interactions.ts:494`), in amber/dashed, driven by a +small store slice `highlight: { ids, tone }`. ~30 lines mirroring `syncSelection`. + +The assistant **never calls `setSelection`**. Selection is the user's pointer; +`interactions.ts:296` uses it for real editing gestures, and taking it +mid-conversation destroys the context the answer was about. The assistant may +*propose* "select these". + +Every answer carries a **focus set** distinct from its inline citations, rendered +as "Show on drawing" — highlights the whole set and fits the view to its bounding +box. When `focusOrder` is true (causal chains like PT-101 → PIC-101 → PV-101) it +flashes them in sequence, which is more legible than the prose and costs an array +of ids. + +## 11. Ghost preview + +Feasible, and built. The naive approach fails for a known reason: `reconcile` +ends with a sweep removing every cell not backed by the document +(`src/canvas/reconciler.ts:65-68`), so ghost *cells* die on the next store change. + +But the app never needed cells for overlays. Four working precedents create raw +SVG inside `.joint-layers` — the group carrying the pan/zoom transform, so the +overlay sits on the sheet, not the viewport: `renderSheet` +(`src/canvas/paperSetup.ts:66`), `renderUnderlay` (`src/canvas/underlay.ts:15`), +`showDockHint` (`src/canvas/autoConnect.ts:158`), and the alignment guides +(`src/canvas/interactions.ts:326`). All are `pointer-events: none`, all invisible +to the reconciler, all survive every store update. + +Geometry is free: `SymbolDef.render(cfg)` returns SVG +(`src/symbols/types.ts:42`), which is what `markupFor` already feeds JointJS. A +`renderGhosts(paper, proposed)` calls the same renderer at the same `dimsFor` box +and gets pixel-identical geometry at `opacity: .45`, dashed. Deletions get a red +halo rect at `cell.getBBox()`. + +**Two non-negotiable details:** + +- **Add `.pid-ghost` to the export strip list** (`src/export/svg.ts:60-69`, + alongside `.pid-underlay` and `.pid-sheet`). Miss this and a *rejected* + proposal ships inside a delivered SVG or PDF. That is the worst bug this + feature can have. +- Ghosts never enter `doc`, so autosave and cloud sync cannot see them. That + falls out for free, which is how we know the design is right. + +## 12. Transport + +```ts +// src/assist/transport/types.ts +export interface AssistTransport { + readonly id: string + readonly label: string + /** Mirrors firebaseReady (src/auth/config.ts:38): the UI asks before offering. */ + isConfigured(): boolean + send(req: AssistRequest, signal: AbortSignal): AsyncIterable +} +``` + +`send` yields **provider-neutral events**, never raw SSE. All wire-format +knowledge is confined behind the iterator, so swapping BYOK for a hosted proxy is +one line in `pickTransport()`. Degrades the way the rest of the app does +(`src/auth/config.ts:33-40`): key present → BYOK; else `firebaseReady` → proxy; +else the panel says the assistant isn't configured and the app is untouched. + +## 13. UI placement + +**A dock in the existing right column, mutually exclusive with the property +panel, switched by a segmented control above it.** + +Rejected, with reasons: + +- **A 5th rail workspace** is disqualified by code, not taste. Every non-`draw` + workspace unmounts `` (`src/EditorRoot.tsx:35-43`), which unmounts + ``, whose cleanup calls `paper.remove()` (`src/canvas/Canvas.tsx:191`). + **A rail workspace and a ghost preview are mutually exclusive.** +- **The command-palette overlay** dims and blocks the whole app + (`src/app.css:191`) and closes on any outside click + (`src/panels/CommandPalette.tsx:121`). Ctrl+K should *summon* the dock and get + out of the way — reuse the entry point, not the container. +- **The Drawer** is `max-height: 190px; flex: none` (`src/app.css:139`) and + horizontal; a conversation with consent cards and diff tables is vertical. A + third tab also re-creates the three-overlapping-lists problem plan §3.4 + deliberately collapsed into two. +- **A floating panel** covers the canvas — exactly the region the ghost preview + needs — and there is no floating-window infrastructure here. + +Column arithmetic decides the rest: rail 60 + palette 236 + props 288 + a new +320px dock = 904px of chrome, leaving 376px of canvas on a 1280 laptop +(`src/app.css:30-43`). Sharing the 288px props column costs **zero** new pixels +and inherits the collapse strip that already exists. Both are right-hand +inspectors, and while reading a consent card you are not editing a property field. + +The toolbar earns exactly **one** control — an icon toggle at the existing 26px +geometry. Everything else lives in the dock and on Ctrl+K. + +## 14. Privacy and licensing + +**Two published claims break the moment this ships and must be rewritten in the +same release:** + +- `SECURITY.md:31` — *"There is no backend, no account system, and no server that + stores user data"* (already stale since v0.13 accounts; decisively false once + drawing content reaches an inference API). +- `README.md:106` — *"Works offline — the whole editor runs with no internet."* + The assistant must therefore degrade to **absent** — no entry point, no error + toast. + +**Consent before the first request:** a one-time modal at first use, not at boot, +using the existing `src/panels/Modal.tsx`. Two checkboxes, not one — *"I +understand my drawing content is sent to \"* and *"I am authorised to +send this drawing outside my organisation."* The second matters: the users are +consultants and EPC staff whose drawings are frequently under NDA. + +**The assistant sends a projection, never `doc`.** Excluded by default: +`doc.meta.author` and the account email · `doc.customSymbols` (the user's own SVG +IP) · `sheet.underlay` (a DXF underlay is almost always *someone else's* drawing +— the single highest-risk field in the document) · `doc.budget` and `node.cost` · +`cloudId`. + +The control is not a policy document, it is a test: build the projection as an +**explicit allowlist** with a vitest that enumerates `ProjectDoc`'s keys and +**fails when a new field appears unclassified**. That single test is the entire +privacy mechanism; everything else is documentation. + +**Key ownership: BYO, stored in the browser, never in `doc` or Firestore.** A +project-owned key would make the maintainer pay for commercial users' inference — +precisely the use `COMMERCIAL-LICENSE.md` says is not free — and turn the demo +deployment into a free commercial service. The existing env mechanism cannot be +reused: `.env.example` states plainly that the Firebase config ships in the +client bundle and is not a secret. An LLM key emphatically is. + +Consequence to accept openly: **a BYO-key assistant is community tier by +construction and monetises nothing.** Per plan §7.4 the enforceable boundary is +the service, so the paid version is the hosted proxy with entitlement checked +server-side. Both can coexist; the transport interface (§12) keeps that door open. + +## 15. Testing + +Nothing about consent, diffs, ghosts, undo grouping, or redaction needs an LLM — +that is the point of the architecture. One interface, one method, everything else +deterministic. Production is HTTP; tests get a `ScriptedTransport` fed fixture +turns. + +Vitest, following `tests/store/budget.test.ts:9`'s existing setup pattern: + +- Every tool against the real store, positive and negative. +- **The redaction allowlist test** that fails on a new unclassified `ProjectDoc` + field — the most valuable single test in the feature. +- Diff builder: tool args + doc → expected rows, no model involved. +- **Undo grouping:** apply a fix, assert `temporal.getState().pastStates.length` + grew by exactly 1, assert one `undo()` restores the prior doc. +- Denial: a scripted transcript where a tool is denied; assert `doc` identity is + unchanged and the reason reached the next turn. +- **The critical negative test:** a transcript where the model is denied and then + asserts the fact anyway. The assertion is that the grounding validator + *rejects the message* — not that the prose looks plausible. This encodes "may + only assert facts a tool returned." + +Playwright via `window.__pid` (`src/main.tsx:52`) plus a dev-only transport seam +guarded by `import.meta.env.DEV`, the same technique as the `pid.dev.skipAuth` +bypass (`src/EditorRoot.tsx:91`). Assert: card renders the right diff · Deny +leaves `doc` identity unchanged · one Ctrl+Z reverts a fix · `.pid-ghost` exists +before approval and is gone after · **and never appears in `exportSvg()` output**. + +Golden transcripts under `tests/fixtures/assistant/`, per plan §7.5. Recorded +once, replayed forever, re-recorded deliberately and reviewed like code. One live +smoke test behind an env var, never in CI. + +## 16. Sequencing + +| Release | Ships | Standalone value | +|---|---|---| +| **v0.17.0** | The QA engine (index, rules, engine, ignores — largely built already, see §1), the fix-representation change (§5), and the five bug fixes below | 21 rules with auditable ignores; cross-sheet selection finally works; engineering records stop being stranded; validation walks the document once instead of three times | +| **v0.18.0** | Assistant v1 — brief, template layer, 6 read tools, 3 fixes, grounding validator, consent, ghosts, dock, BYOK | "The Advisor you can talk to" | +| **v0.19.0+** | The remaining QA rules (plan §4.2), the other 7 fixes, destructive tools with blast-radius cards, hosted proxy | — | + +### The five bugs, landing in v0.17.0 + +1. **`setTag` discards the `collision` flag.** `retagRegistry` returns + `{ registry, collision }` (`src/model/registry.ts:95`) — the flag exists + because *"silently merging two engineering records is unrecoverable"* — but + `setTag` destructures only `registry` (`src/store/store.ts:326`). Retag onto an + occupied tag and the record is silently stranded. +2. **Cross-sheet selection is invisible.** `LoopPanel` selects project-wide ids + (`src/panels/LoopPanel.tsx:22`); `syncSelection` only iterates the active + sheet's cells (`src/canvas/interactions.ts:495`). +3. **`PropertyPanel` can't inspect an off-sheet selection** — it looks the id up + only in `activeSheet` (`src/panels/PropertyPanel.tsx:293`). Same root cause. +4. **`applyFix` returns `void`** and silently no-ops (`src/assist/fixes.ts:31`). + Must return `{ ok, changedIds, message }`. +5. **`locateCell` can't cross sheets**, so `AdvisorPanel` works around it with + `setTimeout(…, 50)` (`src/panels/AdvisorPanel.tsx:39`). Give it a `sheetId` + parameter that switches and retries on the next reconcile tick. + +Bugs 2, 3 and 5 are **prerequisites** for the user's "highlight the selected +loop" requirement, not incidental cleanup. + +The first implementation plan covers **v0.17.0 only**. v0.18.0 gets its own plan +once the foundations are on `main` and `buildIndex`'s shape has settled — the +selection brief is a projection of it, and specifying that projection against an +unbuilt index is how the two drift. + +## 17. Out of scope + +Stated as decisions so they are not relitigated. + +| Not this | Why | +|---|---| +| A generic `applyPatch` / `setDoc` tool | A typed boundary with one generic tool is not a boundary | +| Assistant access to `loadIntoStore`, export, cloud sync, or account | Clears undo history and unlinks the cloud drawing (`src/store/store.ts:650`) | +| Destructive tools in v1 | Consent cards for deletion are the ones we would get wrong first, and getting them wrong once costs the trust the whole feature runs on | +| Autonomous or background runs | Nothing acts without a human in the foreground | +| "Auto-fix all findings" as one button | The unit of consent is one `FixSpec`. Seven fixes is seven dialogs or a reviewed multi-select list | +| Assistant-driven tag renumbering | Plan §9: *"Silently rewriting engineering identities is the fastest way to lose a user's trust"* | +| Persisted always-allow | A permission granted in a context nobody remembers is how manual mode silently becomes auto mode | +| Selections larger than 8 objects reasoned over individually | Aggregate counts only | +| Cross-sheet loop reasoning before bugs 2/3 are fixed | Highlighting half a loop and silently hiding the rest is worse than declining | +| Hazard analysis, relief sizing, SIS/trip logic | Questions 5, 7, 18. An assistant that answers these fluently is a professional liability | +| HMI tools | An entire second surface with no v1 value | +| Conversation memory across sessions | — | +| The word "prediction" | Replaced by two mechanical predictors that need no model: typical-completion from `TYPICALS`, and finding-delta against a hypothetical finished state. Both are what the user actually asked for | + +--- + +*Grounded in an audit of IPD Studio v0.16.0 at `be431e9`, with figures measured +against `examples/sample-refinery-unit.pnid.json`.* diff --git a/e2e/autoconnect.spec.ts b/e2e/autoconnect.spec.ts index 1c84d32..421488e 100644 --- a/e2e/autoconnect.spec.ts +++ b/e2e/autoconnect.spec.ts @@ -3,8 +3,12 @@ import { expect, test, type Page } from '@playwright/test' /* Magnetic docking — connect by touching, not by drawing: - drop a palette symbol onto an existing symbol's connection point and it clicks into place already connected - - drag a symbol already on the sheet the same way, in one undo step - - pull the pair apart afterwards and the pipe stretches instead of breaking */ + - drag a symbol already on the sheet the same way: the line is drawn the + moment the points meet, WITHOUT releasing the mouse, and keeping the drag + going stretches the pipe + - the symbol stands off from the point it landed on, so the pipe is visible + instead of hidden behind two touching symbols + - shaking the symbol mid-drag cuts the line that drag just made */ interface PidHook { useStore: { getState(): any; temporal: { getState(): any } } @@ -28,6 +32,17 @@ async function drag(page: Page, from: { x: number; y: number }, to: { x: number; await page.mouse.up() } +/** Waggle the pointer where it is — the "no, not there" gesture. */ +async function shakePointer(page: Page, at: { x: number; y: number }) { + for (let i = 0; i < 6; i++) await page.mouse.move(at.x + (i % 2 ? -45 : 45), at.y) +} + +/** Tuple form of clientPoint, to spread straight into page.mouse.move(). */ +async function point(page: Page, x: number, y: number): Promise<[number, number]> { + const p = await clientPoint(page, x, y) + return [p.x, p.y] +} + const sheet = (page: Page) => page.evaluate(() => window.__pid.useStore.getState().doc.sheets[0]) const undoDepth = (page: Page) => page.evaluate(() => window.__pid.useStore.temporal.getState().pastStates.length) @@ -75,8 +90,8 @@ test('dropping a palette symbol on a connection point docks and connects it', as const after = await sheet(page) const dropped = after.nodes.find((n: any) => n.id !== gate) - // pulled into place so the two connection points are the same point - expect({ x: dropped.x, y: dropped.y }).toEqual({ x: 232, y: 200 }) + // pulled into line with the nozzle and stood off by 24px, so the pipe shows + expect({ x: dropped.x, y: dropped.y }).toEqual({ x: 256, y: 200 }) expect(after.edges).toHaveLength(1) expect(after.edges[0].source).toEqual({ nodeId: dropped.id, portId: 'w' }) expect(after.edges[0].target).toEqual({ nodeId: gate, portId: 'e' }) @@ -96,7 +111,7 @@ test('a symbol dropped clear of every connection point just lands there', async expect((await sheet(page)).edges).toHaveLength(0) }) -test('dragging a placed symbol onto a connection point docks it, and pulling away stretches the line', async ({ page }) => { +test('a drag connects the moment the points meet, and goes on stretching the pipe', async ({ page }) => { const gate = await withGateValve(page) const moving = await page.evaluate(() => { const s = window.__pid.useStore.getState() @@ -107,35 +122,76 @@ test('dragging a placed symbol onto a connection point docks it, and pulling awa await expect(page.locator('[model-id]')).toHaveCount(2) const before = await undoDepth(page) - // Grab the moving valve by its centre (416,308) and bring its w port to - // within a few px of the fixed valve's e port at (232,208). - await drag(page, await clientPoint(page, 416, 308), await clientPoint(page, 254, 208)) + // Grab the moving valve by its centre and bring its w port up to the fixed + // valve's e port at (232,208) — WITHOUT releasing the button. + await page.mouse.move(...(await point(page, 416, 308))) + await page.mouse.down() + await page.mouse.move(...(await point(page, 254, 208)), { steps: 16 }) + // connected already, mid-drag, with the button still down await expect.poll(async () => (await sheet(page)).edges.length).toBe(1) - const docked = await sheet(page) - const node = docked.nodes.find((n: any) => n.id === moving) - expect({ x: node.x, y: node.y }).toEqual({ x: 232, y: 200 }) - expect(docked.edges[0].source).toEqual({ nodeId: moving, portId: 'w' }) - expect(docked.edges[0].target).toEqual({ nodeId: gate, portId: 'e' }) - // the move and the line are one gesture, so they are one undo step - expect(await undoDepth(page)).toBe(before + 1) + const caught = await sheet(page) + expect(caught.edges[0].source).toEqual({ nodeId: moving, portId: 'w' }) + expect(caught.edges[0].target).toEqual({ nodeId: gate, portId: 'e' }) + // stood off, so there is a visible pipe rather than two symbols touching + expect(caught.nodes.find((n: any) => n.id === moving)).toMatchObject({ x: 256, y: 200 }) - // docked: the ports coincide, so there is no pipe drawn between them yet - const edgeId = docked.edges[0].id + const edgeId = caught.edges[0].id const length = () => page.evaluate((id) => { const paper = window.__pid.canvasRef.paper! const view = paper.model.getCell(id).findView(paper) return view.getConnectionLength() as number }, edgeId) - await expect.poll(length).toBeLessThan(2) + // a real, visible run of pipe — not two symbols touching with nothing drawn + await expect.poll(length).toBeGreaterThan(10) + await expect.poll(length).toBeLessThan(32) + + // keep dragging — still no mouse-up — and the pipe stretches to follow + await page.mouse.move(...(await point(page, 654, 208)), { steps: 16 }) + await expect.poll(length).toBeGreaterThan(300) + await page.mouse.up() - // pull it away and the pipe stretches to follow instead of breaking - await drag(page, await clientPoint(page, 248, 208), await clientPoint(page, 448, 208)) const moved = await sheet(page) expect(moved.edges).toHaveLength(1) expect(moved.edges[0].source).toEqual({ nodeId: moving, portId: 'w' }) - expect(moved.edges[0].target).toEqual({ nodeId: gate, portId: 'e' }) - expect(moved.nodes.find((n: any) => n.id === moving).x).toBeGreaterThan(400) - await expect.poll(length).toBeGreaterThan(150) + expect(moved.nodes.find((n: any) => n.id === moving).x).toBeGreaterThan(600) + // catch and carry are one gesture, so they are one undo step + expect(await undoDepth(page)).toBe(before + 1) + await page.evaluate(() => window.__pid.useStore.getState().undo()) + const undone = await sheet(page) + expect(undone.edges).toHaveLength(0) + expect(undone.nodes.find((n: any) => n.id === moving)).toMatchObject({ x: 400, y: 300 }) +}) + +test('shaking the symbol mid-drag cuts the line that drag just made', async ({ page }) => { + await withGateValve(page) + const moving = await page.evaluate(() => { + const s = window.__pid.useStore.getState() + const id = s.addNode({ symbolId: 'valve.gate', kind: 'valve', x: 400, y: 300, rotation: 0 }) + s.setSelection([]) + return id as string + }) + await expect(page.locator('[model-id]')).toHaveCount(2) + + await page.mouse.move(...(await point(page, 416, 308))) + await page.mouse.down() + await page.mouse.move(...(await point(page, 254, 208)), { steps: 16 }) + await expect.poll(async () => (await sheet(page)).edges.length).toBe(1) + + // wrong point — waggle it off without ever letting go + const here = await clientPoint(page, 254, 208) + await shakePointer(page, here) + await expect.poll(async () => (await sheet(page)).edges.length).toBe(0) + + // and it does not snap straight back onto the point just rejected, nor + // does letting go sneak the connection back on + await page.mouse.move(...(await point(page, 254, 208)), { steps: 6 }) + expect((await sheet(page)).edges).toHaveLength(0) + await page.mouse.up() + expect((await sheet(page)).edges).toHaveLength(0) + + // one undo still takes the whole gesture back + await page.evaluate(() => window.__pid.useStore.getState().undo()) + expect((await sheet(page)).nodes.find((n: any) => n.id === moving)).toMatchObject({ x: 400, y: 300 }) }) diff --git a/e2e/engineering.spec.ts b/e2e/engineering.spec.ts index fe12c47..02a73da 100644 --- a/e2e/engineering.spec.ts +++ b/e2e/engineering.spec.ts @@ -89,6 +89,7 @@ test('a deleted record surfaces as an orphan and can be purged', async ({ page } const orphan = page.locator('.ws-issue', { hasText: 'engineering record' }) await expect(orphan).toBeVisible() - await orphan.getByRole('button', { name: 'Fix' }).click() + // the fix names what it does rather than saying "Fix" + await orphan.getByRole('button', { name: 'Discard the record' }).click() await expect(page.locator('.ws-issue', { hasText: 'engineering record' })).toHaveCount(0) }) diff --git a/e2e/happy-path.spec.ts b/e2e/happy-path.spec.ts index 22302ee..d534ffc 100644 --- a/e2e/happy-path.spec.ts +++ b/e2e/happy-path.spec.ts @@ -22,13 +22,16 @@ test('place, connect, tag, validate, export', async ({ page }) => { await expect(page.locator('[model-id]')).toHaveCount(4, { timeout: 5000 }) // 3 elements + 1 link // The untagged instrument is flagged - await expect(page.locator('.status')).toContainText('1 finding') + // the untagged instrument is reported (exact counts shift as rules are added, + // so assert the signal, not the number) + await expect(page.locator('.status')).toContainText('finding') // Tag it through the property panel; expansion appears; finding clears await page.locator('.tag-letters').fill('FIC') await page.locator('.tag-loop').fill('101') await expect(page.locator('.tag-expansion')).toHaveText('Flow Indicating Controller') - await expect(page.locator('.status')).toContainText('No findings') + // tagging clears the blocker; remaining advice is not critical + await expect(page.locator('.status')).not.toContainText('critical') // Undo removes the loop digits, redo restores them await page.keyboard.press('ControlOrMeta+z') @@ -50,7 +53,7 @@ test('place, connect, tag, validate, export', async ({ page }) => { // Sample plant loads clean (v1 file exercises schema migration) await page.locator('.tb-template').selectOption('sample') - await expect(page.locator('.status')).toContainText('No findings') + await expect(page.locator('.status')).not.toContainText('critical') await expect(page.locator('.doc-name')).toContainText('Sample Plant') // Multi-sheet: add a sheet, place a symbol there, verify isolation diff --git a/e2e/qa.spec.ts b/e2e/qa.spec.ts new file mode 100644 index 0000000..0c54206 --- /dev/null +++ b/e2e/qa.spec.ts @@ -0,0 +1,80 @@ +import { expect, test, type Page } from '@playwright/test' + +async function seedDuplicate(page: Page) { + await page.goto('/app') + await page.waitForFunction(() => '__pid' in window) + await expect(page.locator('.rail')).toBeVisible() + await page.evaluate(() => { + const { useStore } = (window as never as { __pid: { useStore: { getState(): any } } }).__pid + const s = useStore.getState() + const a = s.addNode({ symbolId: 'instr.bubble', kind: 'instrument', x: 160, y: 160, rotation: 0 }) + const b = useStore.getState().addNode({ symbolId: 'instr.bubble', kind: 'instrument', x: 300, y: 160, rotation: 0 }) + useStore.getState().setTag(a, { letters: 'FT', loop: '101' }) + useStore.getState().setTag(b, { letters: 'FT', loop: '101' }) + }) +} + +test('a duplicate tag is critical, and its fix renumbers the extra one', async ({ page }) => { + await seedDuplicate(page) + await page.getByTestId('rail-checks').click() + + await expect(page.getByTestId('checks-tally')).toContainText('critical') + const group = page.getByTestId('rule-duplicate-tag') + await expect(group).toBeVisible() + + await group.getByRole('button', { name: /next free number/i }).click() + await expect(page.getByTestId('rule-duplicate-tag')).toHaveCount(0) + + const tags = await page.evaluate(() => + (window as never as { __pid: { useStore: { getState(): any } } }).__pid.useStore + .getState().doc.sheets[0].nodes.map((n: any) => `${n.tag.letters}-${n.tag.loop}`).sort(), + ) + expect(new Set(tags).size).toBe(2) +}) + +test('the rail badge counts criticals only', async ({ page }) => { + await seedDuplicate(page) + await expect(page.locator('.rail-badge')).toHaveText('1') +}) + +test('accepting a finding records the reason and survives a reload', async ({ page }) => { + await seedDuplicate(page) + page.on('dialog', (d) => void d.accept('second bubble is an off-page continuation')) + await page.getByTestId('rail-checks').click() + + await page.getByTestId('rule-duplicate-tag').getByRole('button', { name: 'Accept' }).click() + await expect(page.getByTestId('rule-duplicate-tag')).toHaveCount(0) + + // it moves into the accepted section, with the reason kept + await page.getByTestId('checks-ignored').click() + await expect(page.locator('.ws-ignored')).toContainText('off-page continuation') + + // and the acceptance is part of the document, so a reload keeps it. + // Autosave debounces 500ms — wait for the write, or the reload restores a + // snapshot taken before the accept. + await page.waitForTimeout(900) + await page.reload() + await page.waitForFunction(() => '__pid' in window) + await page.getByTestId('rail-checks').click() + await expect(page.getByTestId('rule-duplicate-tag')).toHaveCount(0) + await page.getByTestId('checks-ignored').click() + await expect(page.locator('.ws-ignored')).toContainText('off-page continuation') +}) + +test('a reopened finding comes back', async ({ page }) => { + await seedDuplicate(page) + page.on('dialog', (d) => void d.accept('intentional')) + await page.getByTestId('rail-checks').click() + await page.getByTestId('rule-duplicate-tag').getByRole('button', { name: 'Accept' }).click() + await page.getByTestId('checks-ignored').click() + await page.locator('.ws-ignored').getByRole('button', { name: 'Reopen' }).click() + await expect(page.getByTestId('rule-duplicate-tag')).toBeVisible() +}) + +test('the discipline filter narrows the report', async ({ page }) => { + await seedDuplicate(page) + await page.getByTestId('rail-checks').click() + await expect(page.getByTestId('rule-duplicate-tag')).toBeVisible() + await page.getByTestId('checks-discipline').selectOption('process') + await expect(page.getByTestId('rule-duplicate-tag')).toHaveCount(0) +}) diff --git a/package.json b/package.json index 819d8c6..52bacf0 100644 --- a/package.json +++ b/package.json @@ -1,7 +1,7 @@ { "name": "ipd-studio", "private": true, - "version": "0.16.0", + "version": "0.17.0", "license": "PolyForm-Noncommercial-1.0.0", "type": "module", "scripts": { diff --git a/src/WorkspaceRail.tsx b/src/WorkspaceRail.tsx index 41a28bb..c3225ec 100644 --- a/src/WorkspaceRail.tsx +++ b/src/WorkspaceRail.tsx @@ -5,7 +5,7 @@ import { useEffect } from 'react' import { WORKSPACES, navigateWorkspace, type Workspace } from './routes' import { useStore } from './store/store' -import { issuesFor } from './validate/issues' +import { qaFor } from './validate/engine' interface Entry { icon: string @@ -26,10 +26,10 @@ const ENTRIES: Record = { } export default function WorkspaceRail({ active }: { active: Workspace }) { - // Only actionable counts earn a badge. Suggestions are advice and would cry - // wolf on a drawing that is merely unfinished, so the badge counts hard - // findings alone — the things that would stop a drawing being issued. - const findings = issuesFor(useStore((s) => s.doc)).findings.length + // Only actionable counts earn a badge. Warnings and observations would cry + // wolf on a drawing that is merely unfinished, so the badge counts CRITICALS + // alone — the things that would stop the drawing being issued. + const findings = qaFor(useStore((s) => s.doc)).counts.critical useEffect(() => { const onKey = (e: KeyboardEvent) => { diff --git a/src/app.css b/src/app.css index c16fb7d..1bad95a 100644 --- a/src/app.css +++ b/src/app.css @@ -272,9 +272,22 @@ button { font: inherit; } @keyframes pid-dock-pulse { 50% { fill: rgba(43, 108, 176, 0.38); r: 8.5px; } } -/* The ring still marks the spot, it just stops pulsing. */ +/* Shake-to-cut: a red flash where the line the drag just made used to land. */ +.pid-dock-cut { + fill: none; + stroke: #c53030; + stroke-width: 2; + pointer-events: none; + animation: pid-dock-cut 0.45s ease-out forwards; +} +@keyframes pid-dock-cut { + from { r: 5px; opacity: 1; } + to { r: 18px; opacity: 0; } +} +/* The rings still mark the spot, they just stop moving. */ @media (prefers-reduced-motion: reduce) { .pid-dock-hint { animation: none; } + .pid-dock-cut { animation: none; r: 10px; } } /* --- quick line editor ---------------------------------------------------- */ @@ -619,3 +632,29 @@ button.used-row:hover { background: var(--c-accent-soft); color: var(--c-accent) .eng-field input:focus-visible { outline: 2px solid var(--c-accent); outline-offset: -1px; border-color: var(--c-accent); } .eng-untagged { padding: 10px 2px; font-size: 12px; color: var(--c-ink-2); } .eng-untagged p { margin-bottom: 8px; } + +/* ── QA report: severity sections, ignores ───────────────────────────────── */ +.ws-filter { display: flex; align-items: center; gap: 5px; font-size: 11.5px; color: var(--c-ink-2); } +.ws-filter select { height: 24px; border: 1px solid var(--c-line); border-radius: var(--r); background: var(--c-surface); font: inherit; font-size: 11.5px; } +.ws-sev { margin: 22px 0 8px; max-width: 900px; } +.ws-sev:first-child { margin-top: 0; } +.ws-sev h2 { margin: 0; font-size: 13px; font-weight: 700; } +.ws-sev p { margin: 1px 0 0; font-size: 11.5px; color: var(--c-ink-3); } +.ws-sev.critical h2 { color: var(--c-bad); } +.ws-sev.warning h2 { color: var(--c-warn); } +.ws-sev.info h2 { color: var(--c-accent); } +.ws-group-disc { float: right; font-style: normal; font-size: 9.5px; letter-spacing: .08em; text-transform: uppercase; color: var(--c-ink-3); font-weight: 500; } +.ws-group-why { padding: 5px 10px 6px; font-size: 11.5px; color: var(--c-ink-3); border-bottom: 1px solid var(--c-line-soft); background: var(--c-panel); } +.ws-issue-msg:disabled { cursor: default; color: var(--c-ink-2); } +.ws-issue-msg:disabled:hover { background: none; color: var(--c-ink-2); } +.ws-issue-msg.muted { flex: 1; padding: 7px 10px; font-size: 12.5px; color: var(--c-ink-2); } +.ws-issue-msg.muted em { font-style: normal; color: var(--c-ink-3); } +.ws-issue-ignore { padding: 2px 9px; border: 1px solid var(--c-line); border-radius: 4px; background: var(--c-surface); font: inherit; font-size: 11px; cursor: pointer; color: var(--c-ink-2); } +.ws-issue-ignore:hover { border-color: var(--c-accent); color: var(--c-accent); } +.ws-ignored { margin-top: 24px; max-width: 900px; } +.ws-ignored-head { width: 100%; text-align: left; padding: 7px 10px; border: 1px dashed var(--c-line); border-radius: var(--r); background: none; font: inherit; font-size: 12px; color: var(--c-ink-2); cursor: pointer; } +.ws-ignored-head:hover { border-style: solid; color: var(--c-accent); border-color: var(--c-accent); } +.ws-ignored-head span { color: var(--c-ink-3); } +.ws-ignored .ws-group { margin-top: 8px; } +.drawer-group.sev-critical { color: var(--c-bad); } +.drawer-group.sev-warning { color: var(--c-warn); } diff --git a/src/assist/fixes.ts b/src/assist/fixes.ts index 6cc23b9..3844967 100644 --- a/src/assist/fixes.ts +++ b/src/assist/fixes.ts @@ -3,8 +3,55 @@ // commercial use requires a paid license (see COMMERCIAL-LICENSE.md). import { ulid } from 'ulid' -import type { FixSpec } from '../validate/suggest' -import type { PlantEdge, PlantNode } from '../model/types' + +/** A repair the QA report can apply for the user. Lives here, with the code + * that performs it, rather than with the rules that offer it. */ +export type FixSpec = + | { kind: 'insert-ip'; sheetId: string; edgeId: string } + | { kind: 'purge-record'; key: string } + | { kind: 'assign-tag'; nodeId: string; sheetId: string; letters: string } + +/** What a fix did. A fix that fails silently cannot be trusted by anything + * that reports back to a user — the report, or anything built on it later. */ +export interface FixResult { + ok: boolean + /** Node/edge ids the fix created or changed, for a follow-up jump. */ + changedIds: string[] + message?: string +} + +/** + * What a fix WOULD do, without doing it. One description shared by everything + * that has to ask before acting, so a preview and the action can never drift. + */ +export function describeFix(spec: FixSpec, doc: ProjectDoc): { title: string; blastRadius: string; affectedIds: string[] } { + switch (spec.kind) { + case 'insert-ip': { + const sheet = doc.sheets.find((s) => s.id === spec.sheetId) + return { + title: 'Insert an I/P converter', + blastRadius: `Splits one line on ${sheet?.name ?? 'the sheet'} into two and adds a tagged converter between them.`, + affectedIds: [spec.edgeId], + } + } + case 'purge-record': + return { + title: `Discard the engineering record for ${spec.key}`, + blastRadius: `Deletes ${Object.keys(doc.registry?.[spec.key]?.fields ?? {}).length} stored field(s). Nothing on any sheet carries this key.`, + affectedIds: [], + } + case 'assign-tag': { + const sheet = doc.sheets.find((s) => s.id === spec.sheetId) + return { + title: `Renumber to the next free ${spec.letters} tag`, + blastRadius: `Retags one symbol on ${sheet?.name ?? 'the sheet'}. Its engineering record moves with it.`, + affectedIds: [spec.nodeId], + } + } + } +} + +import type { PlantEdge, PlantNode, ProjectDoc } from '../model/types' import { isPortEnd } from '../model/types' import { portWorld } from '../canvas/alignment' import { isDuplicateTag, nextLoopNumber } from '../isa/autonumber' @@ -13,27 +60,45 @@ import { useStore } from '../store/store' const snap8 = (v: number) => Math.round(v / 8) * 8 /** - * Apply an Advisor fix. insert-ip splits an electric line into - * controller → I/P converter → (pneumatic) valve, placing and tagging the - * converter automatically — one undo step. + * Apply a fix. The single dispatcher — rules describe fixes as data and this is + * the only place that performs one. + * + * insert-ip splits an electric line into controller → I/P converter → + * (pneumatic) valve, placing and tagging the converter automatically. Every + * branch is one undo step. */ -export function applyFix(fix: FixSpec): void { +export function applyFix(fix: FixSpec): FixResult { if (fix.kind === 'purge-record') { + const had = Boolean(useStore.getState().doc.registry?.[fix.key]) useStore.getState().purgeRecord(fix.key) - return + return had + ? { ok: true, changedIds: [] } + : { ok: false, changedIds: [], message: `No record found for ${fix.key}.` } + } + if (fix.kind === 'assign-tag') { + const st = useStore.getState() + if (st.activeSheetId !== fix.sheetId) st.setActiveSheet(fix.sheetId) + const now = useStore.getState() + const exists = now.doc.sheets.some((sh) => sh.nodes.some((n) => n.id === fix.nodeId)) + if (!exists) return { ok: false, changedIds: [], message: 'That symbol is no longer on the drawing.' } + now.setTag(fix.nodeId, { letters: fix.letters, loop: nextLoopNumber(now.doc, fix.letters) }) + return { ok: true, changedIds: [fix.nodeId] } } - if (fix.kind !== 'insert-ip') return const s = useStore.getState() if (s.activeSheetId !== fix.sheetId) s.setActiveSheet(fix.sheetId) const state = useStore.getState() const sheet = state.doc.sheets.find((sh) => sh.id === fix.sheetId) const edge = sheet?.edges.find((e) => e.id === fix.edgeId) - if (!sheet || !edge || !isPortEnd(edge.source) || !isPortEnd(edge.target)) return + if (!sheet || !edge || !isPortEnd(edge.source) || !isPortEnd(edge.target)) { + return { ok: false, changedIds: [], message: 'That line is no longer there to split.' } + } const nodeOf = (id: string) => sheet.nodes.find((n) => n.id === id) const srcNode = nodeOf(edge.source.nodeId) const tgtNode = nodeOf(edge.target.nodeId) - if (!srcNode || !tgtNode) return + if (!srcNode || !tgtNode) { + return { ok: false, changedIds: [], message: 'One end of that line is missing.' } + } const valveEndIsTarget = tgtNode.symbolId.startsWith('cv.') const valve = valveEndIsTarget ? tgtNode : srcNode const sender = valveEndIsTarget ? srcNode : tgtNode @@ -78,4 +143,5 @@ export function applyFix(fix: FixSpec): void { target: { nodeId: valve.id, portId: valveEnd.portId }, } useStore.getState().addBatch([converter], [inEdge, outEdge], [edge.id]) + return { ok: true, changedIds: [converter.id, inEdge.id, outEdge.id] } } diff --git a/src/canvas/locate.ts b/src/canvas/locate.ts new file mode 100644 index 0000000..54f137f --- /dev/null +++ b/src/canvas/locate.ts @@ -0,0 +1,71 @@ +// SPDX-License-Identifier: PolyForm-Noncommercial-1.0.0 +// Copyright © 2026 Praharsh Nagpure — IPD Studio. Noncommercial use only; +// commercial use requires a paid license (see COMMERCIAL-LICENSE.md). + +import type { ProjectDoc } from '../model/types' +import { useStore } from '../store/store' +import { canvasRef } from './paperSetup' + +/** The sheet an id lives on, or null when nothing on any sheet wears it. */ +function sheetOf(doc: ProjectDoc, id: string): string | null { + for (const sheet of doc.sheets) { + if (sheet.nodes.some((n) => n.id === id) || sheet.edges.some((e) => e.id === id)) return sheet.id + } + return null +} + +/** Browsers get a real frame; the node test environment has no rAF. */ +const nextFrame = (cb: () => void): void => { + if (typeof requestAnimationFrame === 'function') requestAnimationFrame(cb) + else setTimeout(cb, 16) +} + +/** ~1/3 s at 60fps. Long enough for React to mount the canvas after a + * workspace switch, short enough that a dead id gives up quietly. */ +const MAX_FRAMES = 20 + +/** A later jump wins: the frame loop of an earlier one stops when it sees the + * generation has moved on, so two quick jumps cannot fight over the viewport. */ +let generation = 0 + +function centre(targetId: string, mine: number, frame: number): void { + if (mine !== generation) return + const { paper, graph } = canvasRef + const cell = graph?.getCell(targetId) + // No canvas yet — a jump from the Checks or Data workspace fires before + // React has mounted it. Wait for it rather than making every caller guess a + // timeout, which is what four of them used to do. + if (!paper || !cell) { + if (frame < MAX_FRAMES) nextFrame(() => centre(targetId, mine, frame + 1)) + return + } + const bbox = cell.getBBox() + const scale = paper.scale().sx + const size = paper.getComputedSize() + paper.translate( + size.width / 2 - (bbox.x + bbox.width / 2) * scale, + size.height / 2 - (bbox.y + bbox.height / 2) * scale, + ) +} + +/** + * Select a cell and centre the canvas on it. + * + * The primitive behind every jump in the app — a QA finding, a report row, a + * command-palette hit, a "where used" reference. It lived in ValidationPanel + * back when the findings list was the only thing that jumped; it is a canvas + * concern, so it lives here now. + * + * It resolves the sheet itself. Selection only ever holds ids on the active + * sheet (see `setSelection`), so jumping to an object on another sheet without + * switching first would select nothing at all — which is how a "where used" + * reference to another sheet used to land on an empty inspector. + */ +export function locateCell(targetId: string | undefined, sheetId?: string): void { + if (!targetId) return + const s = useStore.getState() + const sheet = sheetId ?? sheetOf(s.doc, targetId) + if (sheet && sheet !== s.activeSheetId) s.setActiveSheet(sheet) + useStore.getState().setSelection([targetId]) + centre(targetId, ++generation, 0) +} diff --git a/src/canvas/shapes.ts b/src/canvas/shapes.ts index c4a9782..16a8947 100644 --- a/src/canvas/shapes.ts +++ b/src/canvas/shapes.ts @@ -215,7 +215,7 @@ function toEnd(end: PlantEdge['source']): dia.Link.EndJSON { return isPortEnd(end) ? { id: end.nodeId, port: end.portId } : { x: end.x, y: end.y } } -type Direction = 'left' | 'right' | 'top' | 'bottom' +export type Direction = 'left' | 'right' | 'top' | 'bottom' /** Which way a link should leave a port, from the port's place on its symbol. */ export function portDirection(symbolId: string, portId: string): Direction | null { diff --git a/src/model/projectIndex.ts b/src/model/projectIndex.ts new file mode 100644 index 0000000..3c83c9c --- /dev/null +++ b/src/model/projectIndex.ts @@ -0,0 +1,177 @@ +// SPDX-License-Identifier: PolyForm-Noncommercial-1.0.0 +// Copyright © 2026 Praharsh Nagpure — IPD Studio. Noncommercial use only; +// commercial use requires a paid license (see COMMERCIAL-LICENSE.md). + +import type { PlantEdge, PlantNode, ProjectDoc, Sheet } from './types' +import { isPortEnd } from './types' +import type { EngineeringRecord, EntityKind } from './registry' +import { keyOfEdge, keyOfNode, kindOfNode } from './registry' +import { deriveLoops, type Loop } from '../store/selectors' +import { getSymbol } from '../symbols/registry' +import type { PortKind } from '../symbols/types' + +/** + * One walk of the document, shared by everything that asks questions about it. + * + * Before this, each validation rule re-walked every sheet and rebuilt its own + * maps — `runChecks` alone built four, and the advisor rebuilt a neighbour list + * per node per rule. Twenty-odd rules doing that on every keystroke does not + * scale, and two consumers computing the same thing separately can disagree. + * + * Everything here is derived and read-only. Build it once per document. + */ + +export interface IndexedNode { + node: PlantNode + sheet: Sheet + /** Registry key (the formatted tag), or null when untagged. */ + key: string | null + kind: EntityKind | null + ports: { id: string; kind: PortKind }[] +} + +export interface IndexedEdge { + edge: PlantEdge + sheet: Sheet + /** Registry key (the formatted line number), or null when unnumbered. */ + key: string | null +} + +export interface ProjectIndex { + doc: ProjectDoc + nodes: Map + edges: Map + allNodes: IndexedNode[] + allEdges: IndexedEdge[] + /** Node ids -> the edges touching them. */ + edgesByNode: Map + /** Registry key -> everything on a sheet wearing it (>1 means a duplicate). */ + nodesByKey: Map + edgesByKey: Map + /** Node id -> the node ids it is directly connected to. */ + neighbours: Map + loops: Loop[] + /** Every key currently drawn — a record outside this set is an orphan. */ + liveKeys: Set + records: Record +} + +function portsOf(node: PlantNode): { id: string; kind: PortKind }[] { + try { + return [...getSymbol(node.symbolId).ports, ...(node.extraPorts ?? [])].map((p) => ({ + id: p.id, + kind: p.kind, + })) + } catch { + // an unknown symbol id (a custom symbol not registered yet) has no ports + return [] + } +} + +export function buildIndex(doc: ProjectDoc): ProjectIndex { + const nodes = new Map() + const edges = new Map() + const allNodes: IndexedNode[] = [] + const allEdges: IndexedEdge[] = [] + const edgesByNode = new Map() + const nodesByKey = new Map() + const edgesByKey = new Map() + const neighbours = new Map() + const liveKeys = new Set() + + const push = (map: Map, key: string, value: T) => { + const list = map.get(key) + if (list) list.push(value) + else map.set(key, [value]) + } + + for (const sheet of doc.sheets) { + for (const node of sheet.nodes) { + const key = keyOfNode(node) + const indexed: IndexedNode = { node, sheet, key, kind: kindOfNode(node), ports: portsOf(node) } + nodes.set(node.id, indexed) + allNodes.push(indexed) + if (key) { + push(nodesByKey, key, indexed) + liveKeys.add(key) + } + } + + for (const edge of sheet.edges) { + const key = keyOfEdge(edge) + const indexed: IndexedEdge = { edge, sheet, key } + edges.set(edge.id, indexed) + allEdges.push(indexed) + if (key) { + push(edgesByKey, key, indexed) + liveKeys.add(key) + } + + const ends = [edge.source, edge.target] + for (let i = 0; i < 2; i++) { + const a = ends[i]! + const b = ends[1 - i]! + if (!isPortEnd(a)) continue + push(edgesByNode, a.nodeId, edge) + if (isPortEnd(b) && b.nodeId !== a.nodeId) push(neighbours, a.nodeId, b.nodeId) + } + } + } + + return { + doc, + nodes, + edges, + allNodes, + allEdges, + edgesByNode, + nodesByKey, + edgesByKey, + neighbours, + loops: deriveLoops(doc), + liveKeys, + records: doc.registry ?? {}, + } +} + +/** Edges touching a node. Never allocates for the common empty case. */ +export function edgesOf(ix: ProjectIndex, nodeId: string): PlantEdge[] { + return ix.edgesByNode.get(nodeId) ?? [] +} + +export function neighboursOf(ix: ProjectIndex, nodeId: string): string[] { + return ix.neighbours.get(nodeId) ?? [] +} + +/** The port kind at one end of an edge, or null if the end is free / unknown. */ +export function portKindAt(ix: ProjectIndex, end: PlantEdge['source']): PortKind | null { + if (!isPortEnd(end)) return null + return ix.nodes.get(end.nodeId)?.ports.find((p) => p.id === end.portId)?.kind ?? null +} + +/** + * Nodes reachable from `startId` over signal-family lines within `hops`. + * Used to ask "does this controller drive anything?" without caring how many + * converters and solenoids sit in between. + */ +export function signalReach(ix: ProjectIndex, startId: string, hops: number): Set { + const seen = new Set([startId]) + let frontier = [startId] + for (let i = 0; i < hops && frontier.length; i++) { + const next: string[] = [] + for (const id of frontier) { + for (const e of edgesOf(ix, id)) { + if (!e.lineClass.startsWith('signal') && e.lineClass !== 'link.internal') continue + for (const end of [e.source, e.target]) { + if (isPortEnd(end) && !seen.has(end.nodeId)) { + seen.add(end.nodeId) + next.push(end.nodeId) + } + } + } + } + frontier = next + } + seen.delete(startId) + return seen +} diff --git a/src/model/types.ts b/src/model/types.ts index 2316fbf..8afaa4b 100644 --- a/src/model/types.ts +++ b/src/model/types.ts @@ -7,6 +7,13 @@ import type { HmiScreen } from '../hmi/model' import type { Registry } from './registry' +/** A finding the user has explicitly accepted, with the reason why. */ +export interface IgnoredFinding { + reason: string + by?: string + at: string +} + export type SheetSize = 'A4' | 'A3' | 'A2' | 'A1' | 'ANSI_B' | 'ANSI_D' export type NodeKind = 'equipment' | 'instrument' | 'valve' | 'fitting' | 'annotation' @@ -164,6 +171,9 @@ export interface ProjectDoc { /** Engineering records keyed by tag / line number (v0.15.0+, schemaVersion 5). * See model/registry.ts for why the key is the tag and not the node id. */ registry?: Registry + /** QA state. `ignored` is keyed by RuleFinding.key — rule + engineering key, + * never a node id — so an accepted finding stays accepted across a redraw. */ + qa?: { ignored: Record } } /** Budget settings: display currency, optional target, Lang-style installed @@ -176,13 +186,3 @@ export interface BudgetSettings { overrides?: Record } -/** A validation finding surfaced in the validation panel. */ -export interface Finding { - id: string - checkId: string - message: string - targetId?: string - sheetId?: string - /** Absent = error; 'suggestion' items render in the Advisor tab instead. */ - severity?: 'suggestion' -} diff --git a/src/panels/AdvisorPanel.tsx b/src/panels/AdvisorPanel.tsx deleted file mode 100644 index fd55d4b..0000000 --- a/src/panels/AdvisorPanel.tsx +++ /dev/null @@ -1,60 +0,0 @@ -// SPDX-License-Identifier: PolyForm-Noncommercial-1.0.0 -// Copyright © 2026 Praharsh Nagpure — IPD Studio. Noncommercial use only; -// commercial use requires a paid license (see COMMERCIAL-LICENSE.md). - -import { issuesFor } from '../validate/issues' -import { applyFix } from '../assist/fixes' -import { useStore } from '../store/store' -import { locateCell } from './ValidationPanel' - -const RULE_LABELS: Record = { - 'no-receiver': 'Measurements without a receiver', - 'no-final-element': 'Controllers without a final element', - 'dead-end-instrument': 'Unconnected instruments', - 'needs-ip-converter': 'Signal chain: missing I/P converter', - 'no-relief': 'Vessels without relief', - 'no-fail-position': 'Valve failure positions', - 'valve-tag-on-bubble': 'Tag / symbol mismatches', - 'duplicate-line': 'Duplicate lines', -} - -export function useSuggestions() { - return issuesFor(useStore((s) => s.doc)).suggestions -} - -export default function AdvisorPanel() { - const suggestions = useSuggestions() - const setActiveSheet = useStore((s) => s.setActiveSheet) - if (suggestions.length === 0) { - return
No suggestions — the instrumentation looks complete.
- } - const grouped = new Map() - for (const s of suggestions) { - const list = grouped.get(s.checkId) ?? [] - list.push(s) - grouped.set(s.checkId, list) - } - const go = (s: (typeof suggestions)[number]) => { - if (s.sheetId) setActiveSheet(s.sheetId) - setTimeout(() => locateCell(s.targetId), 50) - } - return ( -
- {[...grouped.entries()].map(([checkId, list]) => ( -
-
{RULE_LABELS[checkId] ?? checkId} ({list.length})
- {list.map((s) => ( -
- - {s.fix && ( - - )} -
- ))} -
- ))} -
- ) -} diff --git a/src/panels/CommandPalette.tsx b/src/panels/CommandPalette.tsx index 6f84ab1..9b209f9 100644 --- a/src/panels/CommandPalette.tsx +++ b/src/panels/CommandPalette.tsx @@ -5,7 +5,7 @@ import { useEffect, useMemo, useRef, useState } from 'react' import { findTag } from '../search/findTag' import { useStore } from '../store/store' -import { locateCell } from './ValidationPanel' +import { locateCell } from '../canvas/locate' import { navigateWorkspace } from '../routes' import { downloadInstrumentIndex, downloadLineList } from '../export/csv' import { canvasRef, fitView } from '../canvas/paperSetup' @@ -98,9 +98,8 @@ export default function CommandPalette() { label: h.display, sub: h.sheetName, run: () => { - setActiveSheet(h.sheetId) navigateWorkspace('draw') - setTimeout(() => locateCell(h.nodeId), 60) + locateCell(h.nodeId, h.sheetId) }, })) : [] diff --git a/src/panels/Drawer.tsx b/src/panels/Drawer.tsx index 2c5f1bf..57671de 100644 --- a/src/panels/Drawer.tsx +++ b/src/panels/Drawer.tsx @@ -6,11 +6,11 @@ import { useState } from 'react' import IssuesPanel from './IssuesPanel' import LoopPanel from './LoopPanel' import { useStore } from '../store/store' -import { issuesFor } from '../validate/issues' +import { qaFor } from '../validate/engine' export default function Drawer() { const [tab, setTab] = useState<'issues' | 'loops' | null>(null) - const { total } = issuesFor(useStore((s) => s.doc)) + const { total } = qaFor(useStore((s) => s.doc)) return (
diff --git a/src/panels/InspectorWhereUsed.tsx b/src/panels/InspectorWhereUsed.tsx index 9d4c283..ea31955 100644 --- a/src/panels/InspectorWhereUsed.tsx +++ b/src/panels/InspectorWhereUsed.tsx @@ -8,7 +8,7 @@ import { formatTag } from '../isa/tag' import { getSymbol } from '../symbols/registry' import { deriveLoops } from '../store/selectors' import { useStore } from '../store/store' -import { locateCell } from './ValidationPanel' +import { locateCell } from '../canvas/locate' interface Ref { key: string diff --git a/src/panels/IssuesPanel.tsx b/src/panels/IssuesPanel.tsx index 5b2cfab..4425ac2 100644 --- a/src/panels/IssuesPanel.tsx +++ b/src/panels/IssuesPanel.tsx @@ -3,58 +3,50 @@ // commercial use requires a paid license (see COMMERCIAL-LICENSE.md). import { useStore } from '../store/store' -import { issuesFor } from '../validate/issues' -import { locateCell } from './ValidationPanel' -import { applyFix } from '../assist/fixes' +import { qaFor } from '../validate/engine' +import { locateCell } from '../canvas/locate' import { navigateWorkspace } from '../routes' +import { applyFix } from '../assist/fixes' /** - * The glance version of the findings, while you draw. - * - * Validation and Advisor used to be two drawer tabs, which meant two lists to - * check and an implicit severity flag deciding which one you were reading. - * They are one engine, so they are one list — errors first, advice under it — - * with the full report a click away in the Checks workspace. + * The glance version while you draw: criticals and warnings only, newest rule + * groups flattened. The full report — information, filters, accept-with-reason + * — lives in the Checks workspace, one click away. */ export default function IssuesPanel() { const doc = useStore((s) => s.doc) - const setActiveSheet = useStore((s) => s.setActiveSheet) - const { findings, suggestions } = issuesFor(doc) + const report = qaFor(doc) - const go = (sheetId: string | undefined, targetId: string | undefined) => { - if (sheetId) setActiveSheet(sheetId) - setTimeout(() => locateCell(targetId), 50) - } + const go = (sheetId?: string, targetId?: string) => locateCell(targetId, sheetId) + + const shown = report.groups.filter((g) => g.rule.severity !== 'info') - if (findings.length === 0 && suggestions.length === 0) { + if (report.total === 0) { return
No findings — the drawing is clean.
} return (
- {findings.length > 0 && ( -
-
Findings ({findings.length})
- {findings.map((f) => ( - - ))} -
+ {shown.length === 0 && ( +
Nothing critical — {report.counts.info} observation{report.counts.info === 1 ? '' : 's'} in Checks
)} - {suggestions.length > 0 && ( -
-
Suggestions ({suggestions.length})
- {suggestions.map((s) => ( -
- - {s.fix && ( - + {shown.map((g) => ( +
+
+ {g.rule.title} ({g.findings.length}) +
+ {g.findings.map((f) => ( +
+ + {f.fix && ( + )}
))}
- )} + ))} diff --git a/src/panels/StatusBar.tsx b/src/panels/StatusBar.tsx index cefdb95..e2bd623 100644 --- a/src/panels/StatusBar.tsx +++ b/src/panels/StatusBar.tsx @@ -3,7 +3,7 @@ // commercial use requires a paid license (see COMMERCIAL-LICENSE.md). import { activeSheet, useStore } from '../store/store' -import { useFindings } from './ValidationPanel' +import { qaFor } from '../validate/engine' import { useCloudStatus } from '../cloud/autosave' import { VersionChip } from './VersionNote' @@ -15,7 +15,7 @@ export default function StatusBar() { const dirty = useStore((s) => s.dirty) const selection = useStore((s) => s.selection) const nodes = useStore((s) => activeSheet(s).nodes.length) - const findings = useFindings() + const qa = qaFor(useStore((s) => s.doc)) const cloud = useCloudStatus() // One line about where the work stands. Two indicators ("Saved" next to @@ -37,8 +37,12 @@ export default function StatusBar() { {selection.length > 0 && {selection.length} selected} - - {findings.length ? `⚠ ${findings.length} finding${findings.length > 1 ? 's' : ''}` : '✓ No findings'} + + {qa.counts.critical + ? `⚠ ${qa.counts.critical} critical` + : qa.total + ? `${qa.total} finding${qa.total > 1 ? 's' : ''}` + : '✓ No findings'} ) diff --git a/src/panels/ValidationPanel.tsx b/src/panels/ValidationPanel.tsx deleted file mode 100644 index 6980845..0000000 --- a/src/panels/ValidationPanel.tsx +++ /dev/null @@ -1,62 +0,0 @@ -// SPDX-License-Identifier: PolyForm-Noncommercial-1.0.0 -// Copyright © 2026 Praharsh Nagpure — IPD Studio. Noncommercial use only; -// commercial use requires a paid license (see COMMERCIAL-LICENSE.md). - -import { issuesFor } from '../validate/issues' -import { useStore } from '../store/store' -import { canvasRef } from '../canvas/paperSetup' - -const CHECK_LABELS: Record = { - 'duplicate-tag': 'Duplicate tags', - 'missing-tag': 'Missing tags', - 'invalid-letters': 'Invalid ISA letters', - 'dangling-end': 'Dangling line ends', - 'incompatible-connection': 'Incompatible connections', - 'duplicate-line-number': 'Duplicate line numbers', -} - -/** Reads the shared per-document pass — see validate/issues.ts for why this - * is not a `useMemo` of its own. */ -export function useFindings() { - return issuesFor(useStore((s) => s.doc)).findings -} - -export function locateCell(targetId: string | undefined) { - if (!targetId) return - useStore.getState().setSelection([targetId]) - const paper = canvasRef.paper - const graph = canvasRef.graph - const cell = graph?.getCell(targetId) - if (!paper || !cell) return - const bbox = cell.getBBox() - const scale = paper.scale().sx - const size = paper.getComputedSize() - paper.translate(size.width / 2 - (bbox.x + bbox.width / 2) * scale, size.height / 2 - (bbox.y + bbox.height / 2) * scale) -} - -export default function ValidationPanel() { - const findings = useFindings() - if (findings.length === 0) { - return
No findings — drawing is clean.
- } - const grouped = new Map() - for (const f of findings) { - const list = grouped.get(f.checkId) ?? [] - list.push(f) - grouped.set(f.checkId, list) - } - return ( -
- {[...grouped.entries()].map(([checkId, list]) => ( -
-
{CHECK_LABELS[checkId] ?? checkId} ({list.length})
- {list.map((f) => ( - - ))} -
- ))} -
- ) -} diff --git a/src/store/store.ts b/src/store/store.ts index 232bd6a..45e0dc0 100644 --- a/src/store/store.ts +++ b/src/store/store.ts @@ -32,8 +32,9 @@ export interface StoreState { moveNodes(ids: string[], dx: number, dy: number): void /** Magnetic docking: drop a component onto another component's * connection point. The move and the line it creates are ONE undo step - * because they are one gesture to the user. */ - dockNode(id: string, x: number, y: number, edge: Omit): void + * because they are one gesture to the user. Returns the new line's id, so + * a shake mid-drag can cut exactly the line that drag just made. */ + dockNode(id: string, x: number, y: number, edge: Omit): string rotateNode(id: string): void setNodeScale(id: string, scale: number): void /** Per-axis stretch (longer horizontal vessel etc.). 1/1 clears all scaling. */ @@ -59,6 +60,10 @@ export interface StoreState { /** Delete a record outright. Only ever called for an orphan the user has * chosen to discard — nothing deletes a record automatically. */ purgeRecord(key: string): void + /** Accept a QA finding, with the reason on the record. Keyed by the finding's + * stable rule+entity key, so it survives deleting and redrawing the symbol. */ + ignoreFinding(key: string, reason: string): void + unignoreFinding(key: string): void setUnderlay(underlay: Sheet['underlay']): void addCustomSymbol(def: CustomSymbolDef): void removeCustomSymbol(id: string): void @@ -221,11 +226,13 @@ export const useStore = create()( }, dockNode(id, x, y, edge) { + const edgeId = ulid() patchSheet((sh) => ({ ...sh, nodes: sh.nodes.map((n) => (n.id === id ? { ...n, x, y } : n)), - edges: [...sh.edges, { ...edge, id: ulid() }], + edges: [...sh.edges, { ...edge, id: edgeId }], })) + return edgeId }, rotateNode(id) { @@ -420,6 +427,31 @@ export const useStore = create()( }) }, + ignoreFinding(key, reason) { + set((s) => ({ + doc: touched({ + ...s.doc, + qa: { + ignored: { + ...s.doc.qa?.ignored, + [key]: { reason, by: s.doc.meta.author || undefined, at: new Date().toISOString() }, + }, + }, + }), + dirty: true, + })) + }, + + unignoreFinding(key) { + set((s) => { + const current = s.doc.qa?.ignored + if (!current?.[key]) return s + const ignored = { ...current } + delete ignored[key] + return { doc: touched({ ...s.doc, qa: { ignored } }), dirty: true } + }) + }, + setMeta(patch) { set((s) => ({ doc: touched({ ...s.doc, meta: { ...s.doc.meta, ...patch } }), dirty: true })) }, diff --git a/src/validate/checks.ts b/src/validate/checks.ts deleted file mode 100644 index 780b9c8..0000000 --- a/src/validate/checks.ts +++ /dev/null @@ -1,106 +0,0 @@ -// SPDX-License-Identifier: PolyForm-Noncommercial-1.0.0 -// Copyright © 2026 Praharsh Nagpure — IPD Studio. Noncommercial use only; -// commercial use requires a paid license (see COMMERCIAL-LICENSE.md). - -import type { Finding, ProjectDoc } from '../model/types' -import { isPortEnd } from '../model/types' -import { validateLetters } from '../isa/tag' -import { formatTag } from '../isa/tag' -import { getSymbol } from '../symbols/registry' -import { canConnect } from '../canvas/connectionRules' -import type { PortKind } from '../symbols/types' - -export function runChecks(doc: ProjectDoc): Finding[] { - const findings: Finding[] = [] - const sheetIds = new Set(doc.sheets.map((sh) => sh.id)) - const nodeIndex = new Map(doc.sheets.flatMap((sh) => sh.nodes.map((n) => [n.id, n] as const))) - - // project-wide duplicate-tag detection - const tagSeen = new Map() - - for (const sheet of doc.sheets) { - const add = (checkId: string, message: string, targetId?: string) => - findings.push({ - id: `${checkId}:${targetId ?? findings.length}`, - checkId, - message, - ...(targetId ? { targetId } : {}), - sheetId: sheet.id, - }) - - // 1. duplicate tags (across all sheets) - for (const node of sheet.nodes) { - if (!node.tag?.letters || !node.tag.loop) continue - const key = formatTag(node.tag, '-') - if (tagSeen.has(key)) add('duplicate-tag', `Tag ${key} appears more than once`, node.id) - else tagSeen.set(key, node.id) - } - - // 2. instruments without a tag - for (const node of sheet.nodes) { - if (node.kind === 'instrument' && (!node.tag || !node.tag.letters || !node.tag.loop)) { - add('missing-tag', `${getSymbol(node.symbolId).name} has no tag`, node.id) - } - } - - // 3. invalid ISA letters - for (const node of sheet.nodes) { - if (!node.tag?.letters) continue - const v = validateLetters(node.tag.letters) - if (!v.ok) add('invalid-letters', `${node.tag.letters}: ${v.reason}`, node.id) - } - - // 4. dangling free ends + 5. incompatible connections - const nodeById = new Map(sheet.nodes.map((n) => [n.id, n])) - const portKind = (end: { nodeId: string; portId: string }): PortKind | null => { - const node = nodeById.get(end.nodeId) - if (!node) return null - return getSymbol(node.symbolId).ports.find((p) => p.id === end.portId)?.kind ?? null - } - for (const edge of sheet.edges) { - for (const end of [edge.source, edge.target]) { - if (!isPortEnd(end)) add('dangling-end', 'Line has an unterminated free end', edge.id) - } - if (isPortEnd(edge.source) && isPortEnd(edge.target)) { - const src = portKind(edge.source) - const tgt = portKind(edge.target) - if (src && tgt && !canConnect(src, tgt, edge.lineClass)) { - add('incompatible-connection', `${edge.lineClass} line connects incompatible ports`, edge.id) - } - } - } - - // 7. off-page connector links (unlinked only matters once there are other sheets) - for (const node of sheet.nodes) { - if (node.symbolId !== 'ann.offpage') continue - if (!node.link) { - if (doc.sheets.length > 1) { - add('unlinked-offpage', 'Off-page connector is not linked to another sheet', node.id) - } - } else if (!sheetIds.has(node.link.sheetId) || !nodeIndex.has(node.link.nodeId)) { - add('broken-link', 'Off-page connector points at a missing sheet or connector', node.id) - } - } - } - - // 6. duplicate line numbers (project-wide) - const lnSeen = new Map() - for (const sheet of doc.sheets) { - for (const edge of sheet.edges) { - const ln = edge.lineNumber - if (!ln || !(ln.size || ln.spec || ln.service || ln.seq)) continue - const key = [ln.size, ln.spec, ln.service, ln.seq].join('-') - if (lnSeen.has(key)) { - findings.push({ - id: `duplicate-line-number:${edge.id}`, - checkId: 'duplicate-line-number', - message: `Line number ${key} appears more than once`, - targetId: edge.id, - sheetId: sheet.id, - }) - } else lnSeen.set(key, edge.id) - } - } - - return findings -} diff --git a/src/validate/engine.ts b/src/validate/engine.ts new file mode 100644 index 0000000..9f2a471 --- /dev/null +++ b/src/validate/engine.ts @@ -0,0 +1,118 @@ +// SPDX-License-Identifier: PolyForm-Noncommercial-1.0.0 +// Copyright © 2026 Praharsh Nagpure — IPD Studio. Noncommercial use only; +// commercial use requires a paid license (see COMMERCIAL-LICENSE.md). + +import type { ProjectDoc } from '../model/types' +import { buildIndex, type ProjectIndex } from '../model/projectIndex' +import type { Rule, RuleFinding, Severity } from './rules' +import { ALL_RULES } from './rules/index' + +export interface IgnoredEntry { + reason: string + by?: string + at: string +} + +export interface ReportGroup { + rule: Rule + findings: RuleFinding[] +} + +export interface QaReport { + /** Live findings, grouped by rule, ordered critical → warning → info. */ + groups: ReportGroup[] + counts: Record + /** Findings the user has explicitly accepted, with their reason. */ + ignored: { finding: RuleFinding; rule: Rule; entry: IgnoredEntry }[] + total: number + index: ProjectIndex +} + +const ORDER: Record = { critical: 0, warning: 1, info: 2 } + +/** + * Run every rule over one index. + * + * A rule that throws is contained: a bad rule must never take the whole report + * down, because then a single edge case would hide every other finding on the + * drawing. It is reported as a finding against itself instead. + */ +export function runRules(ix: ProjectIndex, ignored: Record = {}): QaReport { + const groups: ReportGroup[] = [] + const counts: Record = { critical: 0, warning: 0, info: 0 } + const suppressed: QaReport['ignored'] = [] + + for (const rule of ALL_RULES) { + let produced: RuleFinding[] + try { + produced = rule.run(ix) + } catch (err) { + produced = [ + { + ruleId: rule.id, + key: `__rule-error:${rule.id}`, + entityKey: rule.id, + message: `This check could not run: ${err instanceof Error ? err.message : String(err)}`, + }, + ] + } + + // One finding per (rule, entity). A rule that walks NODES will emit the + // same key twice when two symbols wear one tag — and since the key is the + // engineering identity, those are one entity, so reporting it twice is + // noise and accepting one would silently accept both. The duplicate TAG + // itself is still reported, by the rule whose subject is the duplication. + const live: RuleFinding[] = [] + const seen = new Set() + for (const f of produced) { + if (seen.has(f.key)) continue + seen.add(f.key) + const entry = ignored[f.key] + if (entry) suppressed.push({ finding: f, rule, entry }) + else live.push(f) + } + if (live.length) { + groups.push({ rule, findings: live }) + counts[rule.severity] += live.length + } + } + + groups.sort( + (a, b) => + ORDER[a.rule.severity] - ORDER[b.rule.severity] || + a.rule.discipline.localeCompare(b.rule.discipline) || + a.rule.title.localeCompare(b.rule.title), + ) + + return { + groups, + counts, + ignored: suppressed, + total: counts.critical + counts.warning + counts.info, + index: ix, + } +} + +/** + * One report per document, however many panels ask — the status bar, the rail + * badge, the drawer and the Checks workspace all read this. + */ +let cache: { doc: ProjectDoc; ignored: unknown; value: QaReport } | null = null + +/** A stable stand-in for "no ignores". A fresh `{}` per call would make the + * identity check below always fail, quietly re-running every rule on every + * render — which is the whole thing this cache exists to prevent. */ +const NO_IGNORES: Record = {} + +export function qaFor(doc: ProjectDoc): QaReport { + const ignored = doc.qa?.ignored ?? NO_IGNORES + if (cache && cache.doc === doc && cache.ignored === ignored) return cache.value + const value = runRules(buildIndex(doc), ignored) + cache = { doc, ignored, value } + return value +} + +/** Test seam — the cache would otherwise leak between cases. */ +export function resetQaCache(): void { + cache = null +} diff --git a/src/validate/issues.ts b/src/validate/issues.ts deleted file mode 100644 index f0a9e5d..0000000 --- a/src/validate/issues.ts +++ /dev/null @@ -1,44 +0,0 @@ -// SPDX-License-Identifier: PolyForm-Noncommercial-1.0.0 -// Copyright © 2026 Praharsh Nagpure — IPD Studio. Noncommercial use only; -// commercial use requires a paid license (see COMMERCIAL-LICENSE.md). - -import type { Finding, ProjectDoc } from '../model/types' -import { runChecks } from './checks' -import { runSuggestions, type Suggestion } from './suggest' - -export interface Issues { - /** Hard findings — duplicate tags, illegal letters, dangling ends. */ - findings: Finding[] - /** Advice — severity 'suggestion', never an error. */ - suggestions: Suggestion[] - /** Everything, for the count on the rail and the status bar. */ - total: number -} - -/** - * One pass per document, however many panels ask. - * - * The status bar, the drawer and the validation panel each used to call - * `useMemo(() => runChecks(doc), [doc])` separately, so every keystroke ran the - * checks three times and the suggestions twice — each walking every sheet and - * rebuilding its own maps. Memoising on document identity here collapses that - * to one, and guarantees two panels can never disagree about what is wrong. - * - * A single-entry cache is enough: the store holds exactly one document, and a - * new one replaces the old by identity on every edit. - */ -let cache: { doc: ProjectDoc; value: Issues } | null = null - -export function issuesFor(doc: ProjectDoc): Issues { - if (cache && cache.doc === doc) return cache.value - const findings = runChecks(doc) - const suggestions = runSuggestions(doc) - const value: Issues = { findings, suggestions, total: findings.length + suggestions.length } - cache = { doc, value } - return value -} - -/** Test seam — the cache would otherwise leak between cases. */ -export function resetIssuesCache(): void { - cache = null -} diff --git a/src/validate/rules.ts b/src/validate/rules.ts new file mode 100644 index 0000000..b1d764b --- /dev/null +++ b/src/validate/rules.ts @@ -0,0 +1,76 @@ +// SPDX-License-Identifier: PolyForm-Noncommercial-1.0.0 +// Copyright © 2026 Praharsh Nagpure — IPD Studio. Noncommercial use only; +// commercial use requires a paid license (see COMMERCIAL-LICENSE.md). + +import type { ProjectIndex } from '../model/projectIndex' +import type { FixSpec } from '../assist/fixes' + +/** + * A single engineering check. + * + * Replaces the old split between `runChecks` (errors) and `runSuggestions` + * (advice), which encoded severity as the ABSENCE of a field and could not + * express the middle. A rule now declares what it is, and the report groups by + * that rather than by which function produced it. + */ +export type Severity = 'critical' | 'warning' | 'info' + +/** How an engineer triages, not how the code is organised. */ +export type Discipline = + | 'tagging' + | 'topology' + | 'process' + | 'instrumentation' + | 'data' + +/** + * A repair offered by a rule, as DATA. + * + * Deliberately not a closure: a fix has to be showable before it is run — named + * in a confirmation, previewed with its blast radius, recorded in a revision, + * and replayed in a test. `applyFix(spec)` in assist/fixes.ts is the one place + * that performs one. + */ +export interface Fix { + label: string + spec: FixSpec +} + +export interface RuleFinding { + ruleId: string + /** + * Stable identity for this finding: the rule plus the ENGINEERING key of + * what it is about, never a node id. That is what lets an "ignore" survive + * deleting and redrawing the symbol, and lets a finding be tracked from one + * revision to the next. + */ + key: string + /** Tag, line number, or a synthetic id when the subject has no key yet. */ + entityKey: string + message: string + /** Node or edge id, so the report can jump to it. */ + targetId?: string + sheetId?: string + fix?: Fix +} + +export interface Rule { + id: string + /** Group heading in the report — plain engineering language. */ + title: string + severity: Severity + discipline: Discipline + /** One-line statement of why this matters, shown under the group. */ + why?: string + run(ix: ProjectIndex): RuleFinding[] +} + +/** Helper for rules: build a finding with the key convention applied. */ +export function finding( + rule: Pick, + entityKey: string, + message: string, + extra: { targetId?: string; sheetId?: string; fix?: Fix } = {}, +): RuleFinding { + return { ruleId: rule.id, key: `${rule.id}:${entityKey}`, entityKey, message, ...extra } +} diff --git a/src/validate/rules/data.ts b/src/validate/rules/data.ts new file mode 100644 index 0000000..2a15ce9 --- /dev/null +++ b/src/validate/rules/data.ts @@ -0,0 +1,112 @@ +// SPDX-License-Identifier: PolyForm-Noncommercial-1.0.0 +// Copyright © 2026 Praharsh Nagpure — IPD Studio. Noncommercial use only; +// commercial use requires a paid license (see COMMERCIAL-LICENSE.md). + +import type { Rule } from '../rules' +import { finding } from '../rules' +import type { EntityKind } from '../../model/registry' +import { labelForField } from '../../model/fields' + +/** + * The minimum an object needs before its record means anything. + * + * A built-in default for now. From v0.18 the active company standard supplies + * this list, and the same rule reads it — which is why it is a lookup rather + * than a hardcoded condition. + */ +export const REQUIRED_FIELDS: Record = { + instrument: ['general.service', 'signal.range'], + valve: ['element.size', 'actuation.failPosition'], + equipment: ['general.service'], + line: ['spec.size', 'spec.material'], +} + +export const orphanRecord: Rule = { + id: 'orphan-record', + title: 'Engineering records with nothing on a sheet', + severity: 'warning', + discipline: 'data', + why: 'Deleting a symbol deliberately leaves its record behind. This is where you decide whether to redraw it or discard the record.', + run(ix) { + const out = [] + for (const key of Object.keys(ix.records)) { + if (ix.liveKeys.has(key)) continue + const unassigned = key.startsWith('__unassigned:') + out.push( + finding( + orphanRecord, + key, + unassigned + ? 'A record was imported from an untagged symbol — tag the symbol to reunite them' + : `${key} has an engineering record but nothing on any sheet carries that tag`, + { + fix: { label: 'Discard the record', spec: { kind: 'purge-record', key } }, + }, + ), + ) + } + return out + }, +} + +export const requiredFieldEmpty: Rule = { + id: 'required-field-empty', + title: 'Incomplete engineering records', + severity: 'warning', + discipline: 'data', + why: 'These are the fields a datasheet, an I/O list or a purchase enquiry cannot be produced without.', + run(ix) { + const out = [] + for (const [key, group] of ix.nodesByKey) { + const first = group[0]! + if (!first.kind) continue + const required = REQUIRED_FIELDS[first.kind] + if (!required.length) continue + const record = ix.records[key] + // Only nag about a record someone has STARTED. Firing on every tagged + // object of a drawing that predates the registry buries the report under + // a wall of identical warnings — measured at 11-13 on the bundled + // samples — and an object nobody has begun specifying is not yet an + // omission. Overall completeness is the dashboard's job, not the QA + // report's. + const started = record && Object.values(record.fields).some((v) => v.trim() !== '') + if (!started) continue + const missing = required.filter((f) => { + const value = record?.fields[f] ?? first.node.datasheet?.[f] ?? '' + return value.trim() === '' + }) + if (!missing.length) continue + out.push( + finding(requiredFieldEmpty, key, `${key} is missing ${missing.map(labelForField).join(', ')}`, { + targetId: first.node.id, + sheetId: first.sheet.id, + }), + ) + } + return out + }, +} + +export const equipmentNoRecord: Rule = { + id: 'equipment-no-record', + title: 'Untagged equipment', + severity: 'info', + discipline: 'data', + why: 'Equipment has to be tagged before it can carry a record, appear in the equipment list, or be bought.', + run(ix) { + const out = [] + for (const n of ix.allNodes) { + if (n.kind !== 'equipment' || n.key) continue + const name = n.node.label?.trim() + out.push( + finding(equipmentNoRecord, n.node.id, name ? `"${name}" has no tag, so it carries no record` : 'This equipment has no tag, so it carries no record', { + targetId: n.node.id, + sheetId: n.sheet.id, + }), + ) + } + return out + }, +} + +export const DATA_RULES: Rule[] = [orphanRecord, requiredFieldEmpty, equipmentNoRecord] diff --git a/src/validate/rules/index.ts b/src/validate/rules/index.ts new file mode 100644 index 0000000..7809118 --- /dev/null +++ b/src/validate/rules/index.ts @@ -0,0 +1,22 @@ +// SPDX-License-Identifier: PolyForm-Noncommercial-1.0.0 +// Copyright © 2026 Praharsh Nagpure — IPD Studio. Noncommercial use only; +// commercial use requires a paid license (see COMMERCIAL-LICENSE.md). + +import type { Rule } from '../rules' +import { TAGGING_RULES } from './tagging' +import { TOPOLOGY_RULES } from './topology' +import { INSTRUMENTATION_RULES } from './instrumentation' +import { PROCESS_RULES } from './process' +import { DATA_RULES } from './data' + +/** Every check the engine runs. Order here is irrelevant — the report sorts by + * severity, then discipline, then title. */ +export const ALL_RULES: Rule[] = [ + ...TAGGING_RULES, + ...TOPOLOGY_RULES, + ...INSTRUMENTATION_RULES, + ...PROCESS_RULES, + ...DATA_RULES, +] + +export const RULES_BY_ID: Record = Object.fromEntries(ALL_RULES.map((r) => [r.id, r])) diff --git a/src/validate/rules/instrumentation.ts b/src/validate/rules/instrumentation.ts new file mode 100644 index 0000000..9842618 --- /dev/null +++ b/src/validate/rules/instrumentation.ts @@ -0,0 +1,188 @@ +// SPDX-License-Identifier: PolyForm-Noncommercial-1.0.0 +// Copyright © 2026 Praharsh Nagpure — IPD Studio. Noncommercial use only; +// commercial use requires a paid license (see COMMERCIAL-LICENSE.md). + +import type { Rule } from '../rules' +import { finding } from '../rules' +import { isPortEnd } from '../../model/types' +import { edgesOf, signalReach } from '../../model/projectIndex' +import { formatTag } from '../../isa/tag' + +/** Actuators that need a pneumatic signal, not a milliamp loop. */ +const PNEUMATIC_ACTUATORS = new Set(['diaphragm', 'piston']) + +export const noReceiver: Rule = { + id: 'no-receiver', + title: 'Measurements nothing receives', + severity: 'warning', + discipline: 'instrumentation', + why: 'A transmitter with no indicator or controller in its loop measures something nobody reads.', + run(ix) { + const out = [] + for (const n of ix.allNodes) { + const t = n.node.tag + if (n.node.kind !== 'instrument' || !t?.letters || !t.loop) continue + if (t.letters.length < 2 || !t.letters.endsWith('T')) continue + const family = t.letters[0] + const hasReceiver = ix.allNodes.some((o) => { + const ot = o.node.tag + if (o.node.id === n.node.id || !ot?.letters) return false + if (ot.letters[0] !== family || ot.loop !== t.loop) return false + return /[ICR]/.test(ot.letters.slice(1)) + }) + if (hasReceiver) continue + out.push( + finding(noReceiver, n.key!, `${formatTag(t, '-')} measures but nothing receives it — add an indicator or controller?`, { + targetId: n.node.id, + sheetId: n.sheet.id, + }), + ) + } + return out + }, +} + +export const noFinalElement: Rule = { + id: 'no-final-element', + title: 'Controllers that control nothing', + severity: 'warning', + discipline: 'instrumentation', + why: 'A controller with no final element cannot act on what it measures.', + run(ix) { + const out = [] + for (const n of ix.allNodes) { + const t = n.node.tag + if (n.node.kind !== 'instrument' || !t?.letters || !t.loop) continue + if (!t.letters.includes('C') || t.letters.endsWith('V')) continue + const family = t.letters[0] + const taggedFinal = ix.allNodes.some( + (o) => o.node.kind === 'valve' && o.node.tag?.loop === t.loop && o.node.tag?.letters[0] === family, + ) + const wiredFinal = [...signalReach(ix, n.node.id, 3)].some((id) => ix.nodes.get(id)?.node.kind === 'valve') + if (taggedFinal || wiredFinal) continue + out.push( + finding(noFinalElement, n.key!, `${formatTag(t, '-')} controls nothing — where is its valve?`, { + targetId: n.node.id, + sheetId: n.sheet.id, + }), + ) + } + return out + }, +} + +export const deadEndInstrument: Rule = { + id: 'dead-end-instrument', + title: 'Unconnected instruments', + severity: 'warning', + discipline: 'instrumentation', + why: 'A tagged instrument joined to nothing is either unfinished or left over.', + run(ix) { + const out = [] + for (const n of ix.allNodes) { + if (n.node.kind !== 'instrument' || !n.key) continue + if (edgesOf(ix, n.node.id).length) continue + out.push( + finding(deadEndInstrument, n.key, `${n.key} is not connected to anything`, { + targetId: n.node.id, + sheetId: n.sheet.id, + }), + ) + } + return out + }, +} + +export const noFailPosition: Rule = { + id: 'no-fail-position', + title: 'Control valves without a fail position', + // Warning by default for the same reason as no-relief: it is genuinely + // important, and genuinely unstated on plenty of early-stage drawings. The + // company standard is what turns it into a blocker. + severity: 'warning', + discipline: 'instrumentation', + why: 'What the valve does on loss of signal is a safety decision. It cannot be left unstated — and it is not one software should guess, so there is no auto-fix here on purpose.', + run(ix) { + const out = [] + for (const n of ix.allNodes) { + if (!n.node.symbolId.startsWith('cv.')) continue + if ((n.node.config?.fail ?? 'none') !== 'none') continue + out.push( + finding(noFailPosition, n.key ?? n.node.id, `${n.key ?? 'Control valve'} has no failure position (FC / FO / FL)`, { + targetId: n.node.id, + sheetId: n.sheet.id, + }), + ) + } + return out + }, +} + +export const needsIpConverter: Rule = { + id: 'needs-ip-converter', + // A warning rather than an observation: it is a wiring mistake that cannot + // work as drawn, and it is one of the few findings with an exact fix. + title: 'Signal chain: missing I/P converter', + severity: 'warning', + discipline: 'instrumentation', + why: 'A milliamp signal cannot stroke a pneumatic actuator on its own.', + run(ix) { + const out = [] + for (const e of ix.allEdges) { + if (e.edge.lineClass !== 'signal.electric') continue + const { source, target } = e.edge + if (!isPortEnd(source) || !isPortEnd(target)) continue + for (const [end, other] of [[source, target], [target, source]] as const) { + const valve = ix.nodes.get(end.nodeId) + const sender = ix.nodes.get(other.nodeId) + if (!valve || !sender) continue + if (!valve.node.symbolId.startsWith('cv.') || end.portId !== 'sig') continue + if (!PNEUMATIC_ACTUATORS.has(valve.node.config?.actuator ?? 'diaphragm')) continue + if (sender.node.symbolId === 'instr.converter') continue + const sheetId = e.sheet.id + const edgeId = e.edge.id + out.push( + finding(needsIpConverter, e.key ?? edgeId, `Electric signal drives ${valve.key ?? 'a control valve'} with a pneumatic actuator — insert an I/P converter`, { + targetId: edgeId, + sheetId, + fix: { label: 'Insert an I/P converter', spec: { kind: 'insert-ip', sheetId, edgeId } }, + }), + ) + } + } + return out + }, +} + +export const notInLoop: Rule = { + id: 'instrument-not-in-loop', + title: 'Instruments outside any loop', + severity: 'info', + discipline: 'instrumentation', + why: 'An instrument that shares its loop number with nothing else gets no loop diagram.', + run(ix) { + const out = [] + for (const loop of ix.loops) { + if (loop.members.length !== 1) continue + const only = loop.members[0]! + const n = ix.nodes.get(only.nodeId) + if (!n || n.node.kind !== 'instrument') continue + out.push( + finding(notInLoop, n.key ?? only.nodeId, `${formatTag(only.tag, '-')} is the only instrument on loop ${loop.family}-${loop.loop}`, { + targetId: only.nodeId, + sheetId: n.sheet.id, + }), + ) + } + return out + }, +} + +export const INSTRUMENTATION_RULES: Rule[] = [ + noFailPosition, + noReceiver, + noFinalElement, + deadEndInstrument, + needsIpConverter, + notInLoop, +] diff --git a/src/validate/rules/process.ts b/src/validate/rules/process.ts new file mode 100644 index 0000000..c07542a --- /dev/null +++ b/src/validate/rules/process.ts @@ -0,0 +1,69 @@ +// SPDX-License-Identifier: PolyForm-Noncommercial-1.0.0 +// Copyright © 2026 Praharsh Nagpure — IPD Studio. Noncommercial use only; +// commercial use requires a paid license (see COMMERCIAL-LICENSE.md). + +import type { Rule } from '../rules' +import { finding } from '../rules' +import { edgesOf, neighboursOf } from '../../model/projectIndex' + +const RELIEF_SYMBOLS = new Set(['psv', 'pse', 'pvsv', 'psv.pilot', 'vacuum-breaker', 'breather', 'flame-arrestor']) + +export const noRelief: Rule = { + id: 'no-relief', + title: 'Vessels without a relief device', + // A real safety concern, but a warning rather than a critical BY DEFAULT: a + // vessel's relief is very often on another sheet or outside the drawing's + // scope, and three of the five bundled sample drawings trip it legitimately. + // A company standard promotes this to critical (v0.18); crying wolf on every + // drawing until then would teach people to ignore the report. + severity: 'warning', + discipline: 'process', + why: 'A vessel that can be blocked in and has no relief path is the classic overpressure case.', + run(ix) { + const out = [] + for (const n of ix.allNodes) { + if (!n.node.symbolId.startsWith('vessel.')) continue + const hasProcess = edgesOf(ix, n.node.id).some( + (e) => e.lineClass.startsWith('process') || e.lineClass.startsWith('pipe'), + ) + if (!hasProcess) continue + const hasRelief = neighboursOf(ix, n.node.id).some((id) => + RELIEF_SYMBOLS.has(ix.nodes.get(id)?.node.symbolId ?? ''), + ) + if (hasRelief) continue + const name = n.node.label || n.key || 'Vessel' + out.push( + finding(noRelief, n.key ?? n.node.id, `${name} has no relief device connected — intended?`, { + targetId: n.node.id, + sheetId: n.sheet.id, + }), + ) + } + return out + }, +} + +export const lineNoService: Rule = { + id: 'line-no-service', + title: 'Numbered lines with no service', + severity: 'warning', + discipline: 'process', + why: 'The service is what tells a reader, and the line list, what is actually in the pipe.', + run(ix) { + const out = [] + for (const e of ix.allEdges) { + if (!e.key) continue + const hasService = Boolean(e.edge.lineNumber?.service?.trim()) || Boolean(e.edge.fluidId) + if (hasService) continue + out.push( + finding(lineNoService, e.key, `Line ${e.key} has no service or fluid assigned`, { + targetId: e.edge.id, + sheetId: e.sheet.id, + }), + ) + } + return out + }, +} + +export const PROCESS_RULES: Rule[] = [noRelief, lineNoService] diff --git a/src/validate/rules/tagging.ts b/src/validate/rules/tagging.ts new file mode 100644 index 0000000..ce1f1f5 --- /dev/null +++ b/src/validate/rules/tagging.ts @@ -0,0 +1,155 @@ +// SPDX-License-Identifier: PolyForm-Noncommercial-1.0.0 +// Copyright © 2026 Praharsh Nagpure — IPD Studio. Noncommercial use only; +// commercial use requires a paid license (see COMMERCIAL-LICENSE.md). + +import type { Rule } from '../rules' +import { finding } from '../rules' +import { formatTag, validateLetters } from '../../isa/tag' +import { getSymbol } from '../../symbols/registry' + +export const duplicateTag: Rule = { + id: 'duplicate-tag', + title: 'Duplicate tags', + severity: 'critical', + discipline: 'tagging', + why: 'Two objects with one tag means the index, the datasheets and the loop all point at the wrong thing.', + run(ix) { + const out = [] + for (const [key, group] of ix.nodesByKey) { + if (group.length < 2) continue + // the first wearer keeps the tag; the rest are the duplicates to resolve + for (const dup of group.slice(1)) { + out.push( + finding(duplicateTag, `${key}#${dup.node.id}`, `${key} is used more than once`, { + targetId: dup.node.id, + sheetId: dup.sheet.id, + fix: dup.node.tag + ? { + label: 'Give it the next free number', + spec: { kind: 'assign-tag', nodeId: dup.node.id, sheetId: dup.sheet.id, letters: dup.node.tag.letters }, + } + : undefined, + }), + ) + } + } + return out + }, +} + +export const missingTag: Rule = { + id: 'missing-tag', + title: 'Untagged instruments', + severity: 'warning', + discipline: 'tagging', + why: 'An untagged instrument cannot appear in the index, carry a record, or join a loop.', + run(ix) { + const out = [] + for (const n of ix.allNodes) { + if (n.node.kind !== 'instrument' || n.key) continue + const name = getSymbol(n.node.symbolId).name + out.push( + finding(missingTag, n.node.id, `${name} has no tag`, { + targetId: n.node.id, + sheetId: n.sheet.id, + }), + ) + } + return out + }, +} + +export const invalidLetters: Rule = { + id: 'invalid-letters', + title: 'Invalid ISA letters', + severity: 'critical', + discipline: 'tagging', + why: 'A tag that does not parse against ISA-5.1 means the drawing says something no reader can act on.', + run(ix) { + const out = [] + for (const n of ix.allNodes) { + // ISA-5.1 letter tables describe INSTRUMENTS. Equipment is tagged by a + // different convention entirely — P-101 for a pump, TK-101 for a tank, + // E-101 for an exchanger — and single-letter prefixes are normal there. + // Running the instrument tables over equipment flags the app's own + // bundled HMI template as an error, which is how this was found. The + // company standard defines the equipment convention (v0.18). + if (n.node.kind !== 'instrument') continue + const letters = n.node.tag?.letters + if (!letters) continue + const v = validateLetters(letters) + if (v.ok) continue + out.push( + finding(invalidLetters, n.key ?? n.node.id, `${letters}: ${v.reason}`, { + targetId: n.node.id, + sheetId: n.sheet.id, + }), + ) + } + return out + }, +} + +export const numberingGap: Rule = { + id: 'tag-numbering-gap', + title: 'Gaps in tag numbering', + severity: 'info', + discipline: 'tagging', + why: 'Usually harmless, occasionally the trace of an instrument that was deleted and never replaced.', + run(ix) { + const byLetters = new Map() + for (const n of ix.allNodes) { + const t = n.node.tag + if (!t?.letters || !t.loop) continue + const num = Number(t.loop) + if (!Number.isFinite(num)) continue + const list = byLetters.get(t.letters) + if (list) list.push(num) + else byLetters.set(t.letters, [num]) + } + const out = [] + for (const [letters, numbers] of byLetters) { + if (numbers.length < 3) continue // too few to call anything a gap + const sorted = [...new Set(numbers)].sort((a, b) => a - b) + const missing: number[] = [] + for (let i = 1; i < sorted.length; i++) { + for (let v = sorted[i - 1]! + 1; v < sorted[i]!; v++) { + missing.push(v) + if (missing.length > 6) break + } + if (missing.length > 6) break + } + if (!missing.length) continue + const shown = missing.slice(0, 6).map((m) => `${letters}-${String(m).padStart(3, '0')}`).join(', ') + out.push( + finding(numberingGap, letters, `${letters} numbering skips ${shown}${missing.length > 6 ? '…' : ''}`), + ) + } + return out + }, +} + +export const valveTagOnBubble: Rule = { + id: 'valve-tag-on-bubble', + title: 'Tag and symbol disagree', + severity: 'info', + discipline: 'tagging', + why: 'A valve tag on an instrument bubble usually means the wrong symbol was placed.', + run(ix) { + const out = [] + for (const n of ix.allNodes) { + const t = n.node.tag + if (n.node.symbolId !== 'instr.bubble' || !t?.letters) continue + if (t.letters.length < 2 || !t.letters.endsWith('V')) continue + out.push( + finding(valveTagOnBubble, n.key ?? n.node.id, `${formatTag(t, '-')} is a valve tag on an instrument bubble — did you mean the valve symbol?`, { + targetId: n.node.id, + sheetId: n.sheet.id, + }), + ) + } + return out + }, +} + +export const TAGGING_RULES: Rule[] = [duplicateTag, invalidLetters, missingTag, numberingGap, valveTagOnBubble] diff --git a/src/validate/rules/topology.ts b/src/validate/rules/topology.ts new file mode 100644 index 0000000..4ea2118 --- /dev/null +++ b/src/validate/rules/topology.ts @@ -0,0 +1,150 @@ +// SPDX-License-Identifier: PolyForm-Noncommercial-1.0.0 +// Copyright © 2026 Praharsh Nagpure — IPD Studio. Noncommercial use only; +// commercial use requires a paid license (see COMMERCIAL-LICENSE.md). + +import type { Rule } from '../rules' +import { finding } from '../rules' +import { isPortEnd } from '../../model/types' +import { portKindAt } from '../../model/projectIndex' +import { canConnect } from '../../canvas/connectionRules' + +export const danglingEnd: Rule = { + id: 'dangling-end', + title: 'Unterminated lines', + severity: 'warning', + discipline: 'topology', + why: 'A line that stops in space carries nothing — the run it belongs to is incomplete.', + run(ix) { + const out = [] + for (const e of ix.allEdges) { + const free = [e.edge.source, e.edge.target].filter((end) => !isPortEnd(end)).length + if (!free) continue + out.push( + finding(danglingEnd, e.key ?? e.edge.id, free === 2 ? 'Line is attached at neither end' : 'Line has an unterminated free end', { + targetId: e.edge.id, + sheetId: e.sheet.id, + }), + ) + } + return out + }, +} + +export const incompatibleConnection: Rule = { + id: 'incompatible-connection', + title: 'Incompatible connections', + severity: 'critical', + discipline: 'topology', + why: 'A signal line into a process nozzle is not a thing that can be built.', + run(ix) { + const out = [] + for (const e of ix.allEdges) { + const { source, target } = e.edge + if (!isPortEnd(source) || !isPortEnd(target)) continue + const a = portKindAt(ix, source) + const b = portKindAt(ix, target) + if (!a || !b || canConnect(a, b, e.edge.lineClass)) continue + out.push( + finding(incompatibleConnection, e.key ?? e.edge.id, `A ${e.edge.lineClass} line connects incompatible ports`, { + targetId: e.edge.id, + sheetId: e.sheet.id, + }), + ) + } + return out + }, +} + +export const duplicateLineNumber: Rule = { + id: 'duplicate-line-number', + title: 'Duplicate line numbers', + severity: 'warning', + discipline: 'topology', + why: 'Two runs sharing a number cannot both be specified, isometric-drawn or tested.', + run(ix) { + const out = [] + for (const [key, group] of ix.edgesByKey) { + if (group.length < 2) continue + for (const dup of group.slice(1)) { + out.push( + finding(duplicateLineNumber, `${key}#${dup.edge.id}`, `Line number ${key} is used more than once`, { + targetId: dup.edge.id, + sheetId: dup.sheet.id, + }), + ) + } + } + return out + }, +} + +export const duplicateParallelLine: Rule = { + id: 'duplicate-parallel-line', + title: 'Doubled lines', + severity: 'info', + discipline: 'topology', + why: 'Two lines between the same two ports draw as one — the extra is invisible and will confuse every downstream count.', + run(ix) { + const out = [] + const seen = new Set() + for (const e of ix.allEdges) { + const { source, target } = e.edge + if (!isPortEnd(source) || !isPortEnd(target)) continue + const key = [`${source.nodeId}:${source.portId}`, `${target.nodeId}:${target.portId}`].sort().join('|') + if (seen.has(key)) { + out.push( + finding(duplicateParallelLine, e.key ?? e.edge.id, 'Two identical lines connect the same two points — delete one?', { + targetId: e.edge.id, + sheetId: e.sheet.id, + }), + ) + } else seen.add(key) + } + return out + }, +} + +export const offpageLink: Rule = { + id: 'offpage-link', + title: 'Off-page connectors', + severity: 'critical', + discipline: 'topology', + why: 'A connector that points nowhere breaks the continuity between sheets that the reader depends on.', + run(ix) { + const out = [] + const multiSheet = ix.doc.sheets.length > 1 + const sheetIds = new Set(ix.doc.sheets.map((s) => s.id)) + for (const n of ix.allNodes) { + if (n.node.symbolId !== 'ann.offpage') continue + const link = n.node.link + const label = n.node.label || 'Off-page connector' + if (!link) { + // on a one-sheet drawing there is nowhere to point yet + if (multiSheet) { + out.push( + finding(offpageLink, n.node.id, `${label} is not linked to another sheet`, { + targetId: n.node.id, + sheetId: n.sheet.id, + }), + ) + } + } else if (!sheetIds.has(link.sheetId) || !ix.nodes.has(link.nodeId)) { + out.push( + finding(offpageLink, n.node.id, `${label} points at a missing sheet or connector`, { + targetId: n.node.id, + sheetId: n.sheet.id, + }), + ) + } + } + return out + }, +} + +export const TOPOLOGY_RULES: Rule[] = [ + incompatibleConnection, + offpageLink, + danglingEnd, + duplicateLineNumber, + duplicateParallelLine, +] diff --git a/src/validate/suggest.ts b/src/validate/suggest.ts deleted file mode 100644 index f196be3..0000000 --- a/src/validate/suggest.ts +++ /dev/null @@ -1,200 +0,0 @@ -// SPDX-License-Identifier: PolyForm-Noncommercial-1.0.0 -// Copyright © 2026 Praharsh Nagpure — IPD Studio. Noncommercial use only; -// commercial use requires a paid license (see COMMERCIAL-LICENSE.md). - -import type { Finding, PlantEdge, PlantNode, ProjectDoc, Sheet } from '../model/types' -import { isPortEnd } from '../model/types' -import { formatTag } from '../isa/tag' -import { liveKeys } from '../model/registry' - -/** A Fix the Advisor can apply for the user. */ -export type FixSpec = - | { kind: 'insert-ip'; sheetId: string; edgeId: string } - | { kind: 'purge-record'; key: string } - -export interface Suggestion extends Finding { - fix?: FixSpec -} - -const RELIEF_SYMBOLS = new Set(['psv', 'pse', 'pvsv', 'psv.pilot', 'vacuum-breaker', 'breather', 'flame-arrestor']) -/** Actuators that need a pneumatic signal, not a milliamp loop. */ -const PNEUMATIC_ACTUATORS = new Set(['diaphragm', 'piston']) - -const tagOf = (n: PlantNode) => (n.tag ? formatTag(n.tag, '-') : null) - -function nodeEnds(sheet: Sheet, nodeId: string): PlantEdge[] { - return sheet.edges.filter( - (e) => - (isPortEnd(e.source) && e.source.nodeId === nodeId) || - (isPortEnd(e.target) && e.target.nodeId === nodeId), - ) -} - -function neighborIds(sheet: Sheet, nodeId: string): string[] { - const out: string[] = [] - for (const e of nodeEnds(sheet, nodeId)) { - for (const end of [e.source, e.target]) { - if (isPortEnd(end) && end.nodeId !== nodeId) out.push(end.nodeId) - } - } - - return out -} - -/** Instruments reachable over signal-family lines within `hops`. */ -function signalReach(sheet: Sheet, startId: string, hops: number): Set { - const seen = new Set([startId]) - let frontier = [startId] - for (let i = 0; i < hops; i++) { - const next: string[] = [] - for (const id of frontier) { - for (const e of nodeEnds(sheet, id)) { - if (!e.lineClass.startsWith('signal') && e.lineClass !== 'link.internal') continue - for (const end of [e.source, e.target]) { - if (isPortEnd(end) && !seen.has(end.nodeId)) { - seen.add(end.nodeId) - next.push(end.nodeId) - } - } - } - } - frontier = next - } - seen.delete(startId) - return seen -} - -/** - * Domain advice: reads the drawing the way an instrument engineer would and - * suggests completions/corrections. Everything here is severity 'suggestion' - * — advice, never an error. - */ -export function runSuggestions(doc: ProjectDoc): Suggestion[] { - const out: Suggestion[] = [] - const allTagged: { node: PlantNode; sheet: Sheet }[] = [] - for (const sheet of doc.sheets) { - for (const node of sheet.nodes) if (node.tag?.letters && node.tag.loop) allTagged.push({ node, sheet }) - } - const add = (checkId: string, sheet: Sheet, message: string, targetId?: string, fix?: FixSpec) => - out.push({ - id: `${checkId}:${targetId ?? out.length}`, - checkId, - message, - severity: 'suggestion', - sheetId: sheet.id, - ...(targetId ? { targetId } : {}), - ...(fix ? { fix } : {}), - }) - - for (const sheet of doc.sheets) { - const byId = new Map(sheet.nodes.map((n) => [n.id, n])) - - for (const node of sheet.nodes) { - const t = node.tag - - // 1. transmitter with no receiver in its loop (project-wide search) - if (t && node.kind === 'instrument' && t.letters.length >= 2 && t.letters.endsWith('T')) { - const family = t.letters[0] - const hasReceiver = allTagged.some(({ node: o }) => { - if (o.id === node.id || !o.tag) return false - if (o.tag.letters[0] !== family || o.tag.loop !== t.loop) return false - return /[ICR]/.test(o.tag.letters.slice(1)) - }) - if (!hasReceiver) { - add('no-receiver', sheet, `${tagOf(node)} measures but nothing receives it — add an indicator or controller?`, node.id) - } - } - - // 2. controller with no final control element - if (t && node.kind === 'instrument' && t.letters.includes('C') && !t.letters.endsWith('V')) { - const family = t.letters[0] - const taggedFinal = allTagged.some( - ({ node: o }) => o.kind === 'valve' && o.tag!.loop === t.loop && o.tag!.letters[0] === family, - ) - const wiredFinal = [...signalReach(sheet, node.id, 3)].some((id) => byId.get(id)?.kind === 'valve') - if (!taggedFinal && !wiredFinal) { - add('no-final-element', sheet, `${tagOf(node)} controls nothing — where is its valve?`, node.id) - } - } - - // 3. tagged instrument connected to nothing at all - if (t && node.kind === 'instrument' && nodeEnds(sheet, node.id).length === 0) { - add('dead-end-instrument', sheet, `${tagOf(node)} is not connected to anything`, node.id) - } - - // 5. vessel with process connections but no relief device attached - if (node.symbolId.startsWith('vessel.')) { - const hasProcess = nodeEnds(sheet, node.id).some((e) => e.lineClass.startsWith('process') || e.lineClass.startsWith('pipe')) - const hasRelief = neighborIds(sheet, node.id).some((id) => RELIEF_SYMBOLS.has(byId.get(id)?.symbolId ?? '')) - if (hasProcess && !hasRelief) { - const name = node.label || tagOf(node) || 'Vessel' - add('no-relief', sheet, `${name} has no relief device connected — intended?`, node.id) - } - } - - // 6. control valve without a declared failure position - if (node.symbolId.startsWith('cv.') && (node.config?.fail ?? 'none') === 'none') { - add('no-fail-position', sheet, `${tagOf(node) ?? 'Control valve'} has no failure position (FC/FO/FL)`, node.id) - } - - // 7. valve letters on an instrument bubble - if (node.symbolId === 'instr.bubble' && t && t.letters.endsWith('V') && t.letters.length >= 2) { - add('valve-tag-on-bubble', sheet, `${tagOf(node)} is a valve tag on an instrument bubble — did you mean the valve symbol?`, node.id) - } - } - - // 8. two lines connecting exactly the same two points - const pairSeen = new Set() - for (const e of sheet.edges) { - if (!isPortEnd(e.source) || !isPortEnd(e.target)) continue - const key = [`${e.source.nodeId}:${e.source.portId}`, `${e.target.nodeId}:${e.target.portId}`].sort().join('|') - if (pairSeen.has(key)) { - add('duplicate-line', sheet, 'Two identical lines connect the same two points — delete one?', e.id) - } else pairSeen.add(key) - } - - // 4. electric signal straight into a pneumatic actuator — offer the I/P fix - for (const e of sheet.edges) { - if (e.lineClass !== 'signal.electric') continue - if (!isPortEnd(e.source) || !isPortEnd(e.target)) continue - const ends = [ - { end: e.source, other: e.target }, - { end: e.target, other: e.source }, - ] - for (const { end, other } of ends) { - const valve = byId.get(end.nodeId) - const sender = byId.get(other.nodeId) - if (!valve || !sender) continue - if (!valve.symbolId.startsWith('cv.') || end.portId !== 'sig') continue - if (!PNEUMATIC_ACTUATORS.has(valve.config?.actuator ?? 'diaphragm')) continue - if (sender.symbolId === 'instr.converter') continue - add( - 'needs-ip-converter', - sheet, - `Electric signal drives ${tagOf(valve) ?? 'a control valve'} with a pneumatic actuator — insert an I/P converter`, - e.id, - { kind: 'insert-ip', sheetId: sheet.id, edgeId: e.id }, - ) - } - } - } - - // 9. engineering records with nothing on any sheet wearing their key. - // Deleting a symbol deliberately leaves its record behind (see deleteIds), so - // this is how the user is told and given the choice to discard it. - const registry = doc.registry - if (registry) { - const live = liveKeys(doc.sheets) - const firstSheet = doc.sheets[0] - for (const key of Object.keys(registry)) { - if (live.has(key)) continue - const unassigned = key.startsWith('__unassigned:') - const message = unassigned - ? 'An engineering record was imported from an untagged symbol — tag the symbol to reunite them' - : `${key} has an engineering record but nothing on any sheet carries that tag` - if (firstSheet) add('orphan-record', firstSheet, message, undefined, { kind: 'purge-record', key }) - } - } - - return out -} diff --git a/src/workspaces/ChecksWorkspace.tsx b/src/workspaces/ChecksWorkspace.tsx index 47bc69e..4cfdbfb 100644 --- a/src/workspaces/ChecksWorkspace.tsx +++ b/src/workspaces/ChecksWorkspace.tsx @@ -2,121 +2,159 @@ // Copyright © 2026 Praharsh Nagpure — IPD Studio. Noncommercial use only; // commercial use requires a paid license (see COMMERCIAL-LICENSE.md). +import { useState } from 'react' import { useStore } from '../store/store' import { navigateWorkspace } from '../routes' -import { issuesFor } from '../validate/issues' -import { locateCell } from '../panels/ValidationPanel' -import { applyFix } from '../assist/fixes' -import type { Finding } from '../model/types' -import type { Suggestion } from '../validate/suggest' +import { qaFor } from '../validate/engine' +import type { Severity } from '../validate/rules' +import { locateCell } from '../canvas/locate' +import { applyFix, describeFix, type FixSpec } from '../assist/fixes' -const LABELS: Record = { - 'duplicate-tag': 'Duplicate tags', - 'missing-tag': 'Missing tags', - 'invalid-letters': 'Invalid ISA letters', - 'dangling-end': 'Dangling line ends', - 'incompatible-connection': 'Incompatible connections', - 'duplicate-line-number': 'Duplicate line numbers', - 'unlinked-offpage': 'Unlinked off-page connectors', - 'broken-link': 'Broken off-page links', - 'no-receiver': 'Measurements without a receiver', - 'no-final-element': 'Controllers without a final element', - 'dead-end-instrument': 'Unconnected instruments', - 'needs-ip-converter': 'Signal chain: missing I/P converter', - 'no-relief': 'Vessels without relief', - 'no-fail-position': 'Valve failure positions', - 'valve-tag-on-bubble': 'Tag / symbol mismatches', - 'duplicate-line': 'Duplicate lines', +const SEVERITY_LABEL: Record = { + critical: 'Critical', + warning: 'Warning', + info: 'Information', } +const SEVERITY_NOTE: Record = { + critical: 'These would stop the drawing being issued.', + warning: 'Worth resolving before issue; not all of them are mistakes.', + info: 'Observations. An engineer may well have meant it this way.', +} + +const DISCIPLINES = ['tagging', 'topology', 'process', 'instrumentation', 'data'] as const + /** - * The findings, full screen, grouped by what they mean rather than by which - * function produced them. The drawer stays as the glance version while you - * draw; this is the one you work through before issuing a drawing. + * The engineering QA report. * - * Severity is still the binary the model can express today — a hard finding or - * advice. The three-level engine with fixes, ignores and standards arrives in - * v0.16; this screen is shaped to receive it. + * Grouped by severity first and discipline second, because an engineer triages + * by "what blocks issue", not by which function produced the finding. Ignored + * items stay visible in their own collapsed section — a hidden ignore rots, and + * the reason someone accepted a finding is exactly what the next reviewer needs. */ export default function ChecksWorkspace() { const doc = useStore((s) => s.doc) - const setActiveSheet = useStore((s) => s.setActiveSheet) - const { findings, suggestions } = issuesFor(doc) + const ignoreFinding = useStore((s) => s.ignoreFinding) + const unignoreFinding = useStore((s) => s.unignoreFinding) + const [discipline, setDiscipline] = useState('all') + const [showIgnored, setShowIgnored] = useState(false) - const go = (item: Finding | Suggestion) => { - if (item.sheetId) setActiveSheet(item.sheetId) + const report = qaFor(doc) + const groups = report.groups.filter((g) => discipline === 'all' || g.rule.discipline === discipline) + + const go = (sheetId?: string, targetId?: string) => { + if (!targetId) return navigateWorkspace('draw') - setTimeout(() => locateCell(item.targetId), 60) + locateCell(targetId, sheetId) + } + + // A fix can fail — the symbol may have moved on since the report was built. + // Saying so beats a button that appears to do nothing. + const runFix = (spec: FixSpec) => { + const result = applyFix(spec) + if (!result.ok) window.alert(result.message ?? 'That fix could not be applied.') } - const group = (items: T[]) => { - const map = new Map() - for (const i of items) { - const list = map.get(i.checkId) ?? [] - list.push(i) - map.set(i.checkId, list) - } - return [...map.entries()] + const accept = (key: string, message: string) => { + const reason = window.prompt(`Accept this finding?\n\n${message}\n\nWhy is it acceptable? (recorded on the drawing)`) + if (reason && reason.trim()) ignoreFinding(key, reason.trim()) } - const clean = findings.length === 0 && suggestions.length === 0 + // one heading per severity, emitted the first time that severity appears + let lastSeverity: Severity | null = null return (

Checks

- - {findings.length ? `${findings.length} finding${findings.length > 1 ? 's' : ''}` : 'No findings'} + + {report.counts.critical + ? `${report.counts.critical} critical` + : report.total === 0 ? 'No findings' : 'Nothing critical'} - {suggestions.length > 0 && ( - {suggestions.length} suggestion{suggestions.length > 1 ? 's' : ''} - )} + {report.counts.warning > 0 && {report.counts.warning} warning} + {report.counts.info > 0 && {report.counts.info} info} + +
- {clean && ( + {report.total === 0 && (

Nothing to fix — every tag parses, every line lands, and the instrumentation reads as complete.

)} - {findings.length > 0 && ( -
-

Findings {findings.length}

-

Errors in the drawing: these would stop it being issued.

- {group(findings).map(([checkId, list]) => ( -
-
{LABELS[checkId] ?? checkId} {list.length}
- {list.map((f) => ( -
- + {groups.map((g) => { + const heading = g.rule.severity !== lastSeverity ? g.rule.severity : null + lastSeverity = g.rule.severity + return ( +
+ {heading && ( +
+

{SEVERITY_LABEL[heading]}

+

{SEVERITY_NOTE[heading]}

+
+ )} +
+
+ {g.rule.title} {g.findings.length} + {g.rule.discipline} +
+ {g.rule.why &&
{g.rule.why}
} + {g.findings.map((f) => ( +
+ + {f.fix && ( + + )} +
))}
- ))} -
- )} +
+ ) + })} - {suggestions.length > 0 && ( -
-

Suggestions {suggestions.length}

-

Advice, never an error — an engineer may well have meant it this way.

- {group(suggestions).map(([checkId, list]) => ( -
-
{LABELS[checkId] ?? checkId} {list.length}
- {list.map((s) => ( -
- - {s.fix && ( - - )} + {report.ignored.length > 0 && ( +
+ + {showIgnored && ( +
+ {report.ignored.map(({ finding: f, entry }) => ( +
+ + {f.message} + — {entry.reason}{entry.by ? ` (${entry.by})` : ''} + +
))}
- ))} -
+ )} +
)}
diff --git a/src/workspaces/DataWorkspace.tsx b/src/workspaces/DataWorkspace.tsx index 091cef3..c48ed54 100644 --- a/src/workspaces/DataWorkspace.tsx +++ b/src/workspaces/DataWorkspace.tsx @@ -5,7 +5,7 @@ import { useState } from 'react' import { useStore } from '../store/store' import { navigateWorkspace } from '../routes' -import { locateCell } from '../panels/ValidationPanel' +import { locateCell } from '../canvas/locate' import { INSTRUMENT_INDEX_COLUMNS, LINE_LIST_COLUMNS, @@ -27,7 +27,6 @@ type Tab = 'instruments' | 'lines' */ export default function DataWorkspace() { const doc = useStore((s) => s.doc) - const setActiveSheet = useStore((s) => s.setActiveSheet) const [tab, setTab] = useState('instruments') const rows = tab === 'instruments' ? instrumentIndexRows(doc) : lineListRows(doc) @@ -35,9 +34,8 @@ export default function DataWorkspace() { // Invariant: every row in every report is a jump, never just text. const jump = (r: ReportRow) => { - setActiveSheet(r.sheetId) navigateWorkspace('draw') - setTimeout(() => locateCell(r.id), 60) + locateCell(r.id, r.sheetId) } return ( diff --git a/tests/assist/fixes.test.ts b/tests/assist/fixes.test.ts index fd8f919..85346eb 100644 --- a/tests/assist/fixes.test.ts +++ b/tests/assist/fixes.test.ts @@ -1,7 +1,8 @@ import { describe, expect, it } from 'vitest' import '../../src/symbols/lib/index' import { applyFix } from '../../src/assist/fixes' -import { runSuggestions } from '../../src/validate/suggest' +import { buildIndex } from '../../src/model/projectIndex' +import { runRules } from '../../src/validate/engine' import { activeSheet, useStore } from '../../src/store/store' import { createEmptyDoc } from '../../src/model/doc' @@ -19,9 +20,11 @@ describe('insert-ip fix', () => { }) s.addEdge({ lineClass: 'signal.electric', source: { nodeId: fic, portId: 's' }, target: { nodeId: fv, portId: 'sig' } }) - const hit = runSuggestions(useStore.getState().doc).find((x) => x.checkId === 'needs-ip-converter') + const hit = runRules(buildIndex(useStore.getState().doc)).groups + .flatMap((g) => g.findings).find((x) => x.ruleId === 'needs-ip-converter') expect(hit?.fix).toBeDefined() - applyFix(hit!.fix!) + const result = applyFix(hit!.fix!.spec) + expect(result.ok).toBe(true) const sheet = activeSheet(useStore.getState()) const conv = sheet.nodes.find((n) => n.symbolId === 'instr.converter') @@ -34,7 +37,7 @@ describe('insert-ip fix', () => { const out = sheet.edges.find((e) => e.lineClass === 'signal.pneumatic')! expect((out.source as { portId: string }).portId).toBe('s') // and the advice clears - expect(runSuggestions(useStore.getState().doc).map((x) => x.checkId)).not.toContain('needs-ip-converter') + expect(runRules(buildIndex(useStore.getState().doc)).groups.map((g) => g.rule.id)).not.toContain('needs-ip-converter') s.loadIntoStore(createEmptyDoc('reset')) }) }) diff --git a/tests/assist/typicals.test.ts b/tests/assist/typicals.test.ts index 105baba..30dd9cf 100644 --- a/tests/assist/typicals.test.ts +++ b/tests/assist/typicals.test.ts @@ -2,7 +2,8 @@ import { describe, expect, it } from 'vitest' import '../../src/symbols/lib/index' import { buildTypical, TYPICALS } from '../../src/assist/typicals' import { createEmptyDoc } from '../../src/model/doc' -import { runChecks } from '../../src/validate/checks' +import { buildIndex } from '../../src/model/projectIndex' +import { runRules } from '../../src/validate/engine' import { getSymbol } from '../../src/symbols/registry' import type { ProjectDoc } from '../../src/model/types' @@ -31,7 +32,8 @@ describe('typical loops', () => { it('placements are valid drawings out of the box', () => { for (const t of TYPICALS) { const doc = docWithTypical(t.id) - expect(runChecks(doc), t.id).toEqual([]) + // what the app places must never be reported as a blocker + expect(runRules(buildIndex(doc)).counts.critical, t.id).toBe(0) } }) it('every edge lands on a real port of a real symbol', () => { diff --git a/tests/validate/checks.test.ts b/tests/validate/checks.test.ts deleted file mode 100644 index eb5893d..0000000 --- a/tests/validate/checks.test.ts +++ /dev/null @@ -1,74 +0,0 @@ -import { describe, expect, it } from 'vitest' -import '../../src/symbols/lib/index' -import { runChecks } from '../../src/validate/checks' -import { createEmptyDoc } from '../../src/model/doc' -import type { PlantEdge, PlantNode, ProjectDoc } from '../../src/model/types' - -let n = 0 -function mk(partial: Partial): PlantNode { - return { id: `n${n++}`, symbolId: 'instr.bubble', kind: 'instrument', x: 0, y: 0, rotation: 0, ...partial } -} -function doc(nodes: PlantNode[], edges: PlantEdge[] = []): ProjectDoc { - const d = createEmptyDoc('t') - d.sheets[0]!.nodes = nodes - d.sheets[0]!.edges = edges - return d -} -const ids = (findings: { checkId: string }[]) => findings.map((f) => f.checkId) - -describe('off-page link checks', () => { - it('unlinked off-page is fine on single-sheet docs, flagged on multi-sheet', () => { - const op = mk({ symbolId: 'ann.offpage', kind: 'annotation' }) - const single = doc([op]) - expect(runChecks(single).map((f) => f.checkId)).not.toContain('unlinked-offpage') - const multi = doc([op]) - multi.sheets.push({ ...multi.sheets[0]!, id: 'sheet2', name: 'Sheet 2', nodes: [], edges: [] }) - expect(runChecks(multi).map((f) => f.checkId)).toContain('unlinked-offpage') - }) - it('flags broken links', () => { - const op = mk({ symbolId: 'ann.offpage', kind: 'annotation', link: { sheetId: 'nope', nodeId: 'gone' } }) - expect(runChecks(doc([op])).map((f) => f.checkId)).toContain('broken-link') - }) -}) - -describe('runChecks', () => { - it('clean doc has no findings', () => { - const a = mk({ tag: { letters: 'FT', loop: '100' } }) - expect(runChecks(doc([a]))).toEqual([]) - }) - it('flags duplicate tags', () => { - const a = mk({ tag: { letters: 'FT', loop: '100' } }) - const b = mk({ tag: { letters: 'FT', loop: '100' } }) - expect(ids(runChecks(doc([a, b])))).toContain('duplicate-tag') - }) - it('flags untagged instruments but not annotations', () => { - const a = mk({}) - const ann = mk({ symbolId: 'ann.text', kind: 'annotation' }) - const findings = runChecks(doc([a, ann])) - expect(findings.filter((f) => f.checkId === 'missing-tag')).toHaveLength(1) - }) - it('flags invalid letter combinations', () => { - const a = mk({ tag: { letters: 'FZZ', loop: '100' } }) - expect(ids(runChecks(doc([a])))).toContain('invalid-letters') - }) - it('flags dangling free ends unless on an off-page connector', () => { - const a = mk({ symbolId: 'pump.centrifugal', kind: 'equipment' }) - const dangling: PlantEdge = { id: 'e1', lineClass: 'process.major', source: { nodeId: a.id, portId: 'discharge' }, target: { x: 50, y: 50 } } - expect(ids(runChecks(doc([a], [dangling])))).toContain('dangling-end') - }) - it('flags incompatible stored connections', () => { - const a = mk({ symbolId: 'pump.centrifugal', kind: 'equipment' }) - const b = mk({ symbolId: 'pump.centrifugal', kind: 'equipment' }) - const bad: PlantEdge = { id: 'e2', lineClass: 'signal.electric', source: { nodeId: a.id, portId: 'discharge' }, target: { nodeId: b.id, portId: 'suction' } } - expect(ids(runChecks(doc([a, b], [bad])))).toContain('incompatible-connection') - }) - it('flags duplicate line numbers', () => { - const a = mk({ symbolId: 'pump.centrifugal', kind: 'equipment' }) - const ln = { size: '2', spec: 'CS150', service: 'P', seq: '001' } - const e1: PlantEdge = { id: 'e3', lineClass: 'process.major', source: { x: 0, y: 0 }, target: { nodeId: a.id, portId: 'suction' }, lineNumber: ln } - const e2: PlantEdge = { id: 'e4', lineClass: 'process.major', source: { x: 0, y: 90 }, target: { nodeId: a.id, portId: 'discharge' }, lineNumber: { ...ln } } - const findings = runChecks(doc([a], [e1, e2])) - expect(ids(findings)).toContain('duplicate-line-number') - expect(ids(findings)).toContain('dangling-end') - }) -}) diff --git a/tests/validate/engine.test.ts b/tests/validate/engine.test.ts new file mode 100644 index 0000000..6c3be8f --- /dev/null +++ b/tests/validate/engine.test.ts @@ -0,0 +1,132 @@ +import { beforeEach, describe, expect, it } from 'vitest' +import '../../src/symbols/lib/index' +import { createEmptyDoc } from '../../src/model/doc' +import { buildIndex } from '../../src/model/projectIndex' +import { runRules, qaFor, resetQaCache } from '../../src/validate/engine' +import { ALL_RULES } from '../../src/validate/rules/index' +import type { PlantEdge, PlantNode, ProjectDoc } from '../../src/model/types' + +let n = 0 +const node = (p: Partial = {}): PlantNode => ({ + id: `n${n++}`, symbolId: 'instr.bubble', kind: 'instrument', x: 0, y: 0, rotation: 0, ...p, +}) +function doc(nodes: PlantNode[], edges: PlantEdge[] = []): ProjectDoc { + const d = createEmptyDoc('t') + d.sheets[0]!.nodes = nodes + d.sheets[0]!.edges = edges + return d +} +const report = (d: ProjectDoc) => runRules(buildIndex(d), d.qa?.ignored ?? {}) +const ids = (d: ProjectDoc) => report(d).groups.flatMap((g) => g.findings.map((f) => f.ruleId)) + +beforeEach(() => resetQaCache()) + +describe('the rule set', () => { + it('every rule has a unique id', () => { + const seen = ALL_RULES.map((r) => r.id) + expect(new Set(seen).size).toBe(seen.length) + }) + + it('every rule declares a severity, a discipline and a reason', () => { + for (const r of ALL_RULES) { + expect(['critical', 'warning', 'info']).toContain(r.severity) + expect(r.discipline).toBeTruthy() + expect(r.title).toBeTruthy() + expect(r.why, `${r.id} should say why it matters`).toBeTruthy() + } + }) +}) + +describe('runRules', () => { + it('a clean drawing reports nothing', () => { + const ft = node({ tag: { letters: 'FT', loop: '100' } }) + const fic = node({ tag: { letters: 'FIC', loop: '100' } }) + const wire: PlantEdge = { + id: 'e1', lineClass: 'signal.electric', + source: { nodeId: ft.id, portId: 'n' }, target: { nodeId: fic.id, portId: 's' }, + } + const d = doc([ft, fic], [wire]) + // only data-completeness advice should remain, never a critical + expect(report(d).counts.critical).toBe(0) + }) + + it('reports duplicate tags as critical, once per extra wearer', () => { + const d = doc([ + node({ tag: { letters: 'FT', loop: '100' } }), + node({ tag: { letters: 'FT', loop: '100' } }), + node({ tag: { letters: 'FT', loop: '100' } }), + ]) + const dup = report(d).groups.find((g) => g.rule.id === 'duplicate-tag') + expect(dup?.rule.severity).toBe('critical') + expect(dup?.findings).toHaveLength(2) + }) + + it('sorts critical before warning before info', () => { + const d = doc([node({}), node({ tag: { letters: 'FZZ', loop: '100' } })]) + const severities = report(d).groups.map((g) => g.rule.severity) + const rank = { critical: 0, warning: 1, info: 2 } as const + const ranks = severities.map((s) => rank[s]) + expect(ranks).toEqual([...ranks].sort((a, b) => a - b)) + }) + + it('a rule that throws is contained, and the rest still run', () => { + const broken = { + id: 'boom', title: 'Boom', severity: 'info' as const, discipline: 'data' as const, why: 'x', + run() { throw new Error('kaboom') }, + } + ALL_RULES.push(broken) + try { + const d = doc([node({ tag: { letters: 'FT', loop: '100' } })]) + const r = report(d) + const boom = r.groups.find((g) => g.rule.id === 'boom') + expect(boom?.findings[0]?.message).toContain('kaboom') + // the report still exists rather than the whole thing failing + expect(r.groups.length).toBeGreaterThan(1) + } finally { + ALL_RULES.pop() + } + }) +}) + +describe('finding identity', () => { + it('emits one finding per rule+entity even when two symbols wear one tag', () => { + // both bubbles are FT-100, so node-walking rules would otherwise produce + // two findings under the identical key + const d = doc([ + node({ tag: { letters: 'FT', loop: '100' } }), + node({ tag: { letters: 'FT', loop: '100' } }), + ]) + const all = report(d).groups.flatMap((g) => g.findings.map((f) => f.key)) + expect(new Set(all).size).toBe(all.length) + }) +}) + +describe('ignores', () => { + const dupDoc = () => doc([ + node({ tag: { letters: 'FT', loop: '100' } }), + node({ tag: { letters: 'FT', loop: '100' } }), + ]) + + it('suppress a finding and move it to the ignored list', () => { + const d = dupDoc() + const key = report(d).groups.find((g) => g.rule.id === 'duplicate-tag')!.findings[0]!.key + d.qa = { ignored: { [key]: { reason: 'second is an off-page continuation', at: '2026-01-01' } } } + const after = report(d) + expect(after.groups.find((g) => g.rule.id === 'duplicate-tag')).toBeUndefined() + expect(after.ignored).toHaveLength(1) + expect(after.ignored[0]!.entry.reason).toContain('off-page') + }) + + it('an unrelated key does not suppress anything', () => { + const d = dupDoc() + d.qa = { ignored: { 'duplicate-tag:nonsense': { reason: 'x', at: '2026-01-01' } } } + expect(ids(d)).toContain('duplicate-tag') + }) +}) + +describe('qaFor caching', () => { + it('returns the identical report for the same document', () => { + const d = doc([node({ tag: { letters: 'FT', loop: '100' } })]) + expect(qaFor(d)).toBe(qaFor(d)) + }) +}) diff --git a/tests/validate/issues.test.ts b/tests/validate/issues.test.ts deleted file mode 100644 index 579af77..0000000 --- a/tests/validate/issues.test.ts +++ /dev/null @@ -1,58 +0,0 @@ -import { beforeEach, describe, expect, it, vi } from 'vitest' -import '../../src/symbols/lib/index' -import { createEmptyDoc } from '../../src/model/doc' -import type { PlantNode, ProjectDoc } from '../../src/model/types' -import { issuesFor, resetIssuesCache } from '../../src/validate/issues' -import * as checks from '../../src/validate/checks' -import * as suggest from '../../src/validate/suggest' - -let n = 0 -const mk = (partial: Partial = {}): PlantNode => ({ - id: `n${n++}`, symbolId: 'instr.bubble', kind: 'instrument', x: 0, y: 0, rotation: 0, ...partial, -}) -function doc(nodes: PlantNode[]): ProjectDoc { - const d = createEmptyDoc('t') - d.sheets[0]!.nodes = nodes - return d -} - -beforeEach(() => resetIssuesCache()) - -describe('issuesFor', () => { - it('reports findings and suggestions together with a combined total', () => { - // untagged instrument -> a finding; nothing connected -> no suggestion for it - const r = issuesFor(doc([mk()])) - expect(r.findings.length).toBeGreaterThan(0) - expect(r.total).toBe(r.findings.length + r.suggestions.length) - }) - - it('returns the identical object for the same document', () => { - const d = doc([mk({ tag: { letters: 'FT', loop: '100' } })]) - expect(issuesFor(d)).toBe(issuesFor(d)) - }) - - // The regression this module exists to prevent: three panels each holding - // their own useMemo ran the checks three times per keystroke. - it('runs each engine once however many callers ask', () => { - const runChecks = vi.spyOn(checks, 'runChecks') - const runSuggestions = vi.spyOn(suggest, 'runSuggestions') - resetIssuesCache() - const d = doc([mk({ tag: { letters: 'FT', loop: '100' } })]) - issuesFor(d) - issuesFor(d) - issuesFor(d) - expect(runChecks).toHaveBeenCalledTimes(1) - expect(runSuggestions).toHaveBeenCalledTimes(1) - runChecks.mockRestore() - runSuggestions.mockRestore() - }) - - it('recomputes when the document changes identity', () => { - const a = doc([mk({ tag: { letters: 'FT', loop: '100' } })]) - const b = doc([mk()]) - const ra = issuesFor(a) - const rb = issuesFor(b) - expect(ra).not.toBe(rb) - expect(rb.findings.length).toBeGreaterThan(ra.findings.length) - }) -}) diff --git a/tests/validate/rules.test.ts b/tests/validate/rules.test.ts new file mode 100644 index 0000000..871336a --- /dev/null +++ b/tests/validate/rules.test.ts @@ -0,0 +1,227 @@ +import { describe, expect, it } from 'vitest' +import '../../src/symbols/lib/index' +import { createEmptyDoc } from '../../src/model/doc' +import { buildIndex } from '../../src/model/projectIndex' +import { runRules } from '../../src/validate/engine' +import { TYPICALS, buildTypical } from '../../src/assist/typicals' +import type { PlantEdge, PlantNode, ProjectDoc, Tag } from '../../src/model/types' + +/** + * Behaviour ported from the retired tests/validate/checks.test.ts and + * suggest.test.ts, restated against the rule engine. Each case names the rule + * it exercises, so a rule that changes severity or id fails loudly here. + */ +let n = 0 +const node = (p: Partial & Pick): PlantNode => ({ + id: `n${n++}`, x: 0, y: 0, rotation: 0, ...p, +}) +const bubble = (tag: Tag, extra?: Partial): PlantNode => + node({ symbolId: 'instr.bubble', kind: 'instrument', tag, ...extra }) + +function docOf(nodes: PlantNode[], edges: PlantEdge[] = []): ProjectDoc { + const d = createEmptyDoc('t') + d.sheets[0]!.nodes = nodes + d.sheets[0]!.edges = edges + return d +} +const ids = (doc: ProjectDoc) => runRules(buildIndex(doc)).groups.flatMap((g) => g.findings.map((f) => f.ruleId)) +const severityOf = (doc: ProjectDoc, ruleId: string) => + runRules(buildIndex(doc)).groups.find((g) => g.rule.id === ruleId)?.rule.severity + +describe('tagging', () => { + // Found by running the rule set over the app's own bundled templates: P-101 + // on a pump was reported as an ISA error. Equipment does not follow the + // instrument letter tables. This false positive existed in the old engine too. + it('invalid-letters: leaves equipment tags alone', () => { + const pump = node({ symbolId: 'pump.centrifugal', kind: 'equipment', tag: { letters: 'P', loop: '101' } }) + expect(ids(docOf([pump]))).not.toContain('invalid-letters') + }) + + it('duplicate-tag: critical, and only the extra wearers are flagged', () => { + const d = docOf([bubble({ letters: 'FT', loop: '100' }), bubble({ letters: 'FT', loop: '100' })]) + expect(severityOf(d, 'duplicate-tag')).toBe('critical') + expect(ids(d).filter((i) => i === 'duplicate-tag')).toHaveLength(1) + }) + + it('missing-tag: flags untagged instruments, never annotations', () => { + const d = docOf([ + node({ symbolId: 'instr.bubble', kind: 'instrument' }), + node({ symbolId: 'ann.text', kind: 'annotation' }), + ]) + expect(ids(d).filter((i) => i === 'missing-tag')).toHaveLength(1) + }) + + it('invalid-letters: rejects an illegal ISA combination', () => { + expect(ids(docOf([bubble({ letters: 'FZZ', loop: '100' })]))).toContain('invalid-letters') + }) + + it('valve-tag-on-bubble: a valve tag on a bubble is an observation', () => { + const d = docOf([bubble({ letters: 'FV', loop: '100' })]) + expect(ids(d)).toContain('valve-tag-on-bubble') + expect(severityOf(d, 'valve-tag-on-bubble')).toBe('info') + }) +}) + +describe('topology', () => { + it('dangling-end: an unattached end is flagged', () => { + const pump = node({ symbolId: 'pump.centrifugal', kind: 'equipment' }) + const e: PlantEdge = { + id: 'e1', lineClass: 'process.major', + source: { nodeId: pump.id, portId: 'discharge' }, target: { x: 50, y: 50 }, + } + expect(ids(docOf([pump], [e]))).toContain('dangling-end') + }) + + it('incompatible-connection: a stored illegal pairing is critical', () => { + // both ends are strictly process ports, so a signal class cannot join them + const pump = node({ symbolId: 'pump.centrifugal', kind: 'equipment' }) + const pump2 = node({ symbolId: 'pump.centrifugal', kind: 'equipment' }) + const bad: PlantEdge = { + id: 'e1', lineClass: 'signal.electric', + source: { nodeId: pump.id, portId: 'discharge' }, target: { nodeId: pump2.id, portId: 'suction' }, + } + const d = docOf([pump, pump2], [bad]) + expect(ids(d)).toContain('incompatible-connection') + expect(severityOf(d, 'incompatible-connection')).toBe('critical') + }) + + it('offpage-link: fine on one sheet, flagged once there are two', () => { + const op = node({ symbolId: 'ann.offpage', kind: 'annotation' }) + const single = docOf([op]) + expect(ids(single)).not.toContain('offpage-link') + + const multi = docOf([op]) + multi.sheets.push({ ...multi.sheets[0]!, id: 'sheet2', name: 'Sheet 2', nodes: [], edges: [] }) + expect(ids(multi)).toContain('offpage-link') + }) + + it('offpage-link: a link pointing nowhere is flagged', () => { + const op = node({ symbolId: 'ann.offpage', kind: 'annotation', link: { sheetId: 'nope', nodeId: 'gone' } }) + expect(ids(docOf([op]))).toContain('offpage-link') + }) + + it('duplicate-line-number: two runs cannot share a number', () => { + const ln = { size: '6"', spec: 'CS', service: 'CW', seq: '001' } + const a: PlantEdge = { id: 'e1', lineClass: 'process.major', source: { x: 0, y: 0 }, target: { x: 9, y: 0 }, lineNumber: ln } + const b: PlantEdge = { id: 'e2', lineClass: 'process.major', source: { x: 0, y: 9 }, target: { x: 9, y: 9 }, lineNumber: ln } + expect(ids(docOf([], [a, b]))).toContain('duplicate-line-number') + }) +}) + +describe('instrumentation', () => { + it('no-receiver: flags a transmitter nobody receives, clears when one exists', () => { + const lt = bubble({ letters: 'LT', loop: '100' }) + expect(ids(docOf([lt]))).toContain('no-receiver') + const lic = bubble({ letters: 'LIC', loop: '100' }) + expect(ids(docOf([lt, lic]))).not.toContain('no-receiver') + }) + + it('no-final-element: flags a controller with no valve, clears when its valve exists', () => { + const fic = bubble({ letters: 'FIC', loop: '100' }) + expect(ids(docOf([fic]))).toContain('no-final-element') + const fv = node({ symbolId: 'cv.globe', kind: 'valve', tag: { letters: 'FV', loop: '100' } }) + expect(ids(docOf([fic, fv]))).not.toContain('no-final-element') + }) + + it('dead-end-instrument: a tagged instrument joined to nothing', () => { + expect(ids(docOf([bubble({ letters: 'PI', loop: '100' })]))).toContain('dead-end-instrument') + }) + + // Warning, not critical, by default — plenty of early-stage drawings leave + // it unstated, and a company standard is what makes it a blocker (v0.18). + it('no-fail-position: a warning, and deliberately offers no auto-fix', () => { + const cv = node({ symbolId: 'cv.globe', kind: 'valve', tag: { letters: 'FV', loop: '100' } }) + const d = docOf([cv]) + expect(severityOf(d, 'no-fail-position')).toBe('warning') + const group = runRules(buildIndex(d)).groups.find((g) => g.rule.id === 'no-fail-position') + expect(group!.findings[0]!.fix).toBeUndefined() + }) + + it('needs-ip-converter: offers the fix for an electric line into a diaphragm valve', () => { + const fic = bubble({ letters: 'FIC', loop: '100' }) + const fv = node({ symbolId: 'cv.globe', kind: 'valve', config: { actuator: 'diaphragm', fail: 'fc' } }) + const wire: PlantEdge = { + id: 'e1', lineClass: 'signal.electric', + source: { nodeId: fic.id, portId: 's' }, target: { nodeId: fv.id, portId: 'sig' }, + } + const d = docOf([fic, fv], [wire]) + const group = runRules(buildIndex(d)).groups.find((g) => g.rule.id === 'needs-ip-converter') + expect(group).toBeDefined() + expect(group!.findings[0]!.fix?.label).toMatch(/I\/P/) + }) + + it('needs-ip-converter: silent on a solenoid valve', () => { + const fic = bubble({ letters: 'FIC', loop: '100' }) + const xv = node({ symbolId: 'valve.solenoid', kind: 'valve' }) + const wire: PlantEdge = { + id: 'e1', lineClass: 'signal.electric', + source: { nodeId: fic.id, portId: 's' }, target: { nodeId: xv.id, portId: 'sig' }, + } + expect(ids(docOf([fic, xv], [wire]))).not.toContain('needs-ip-converter') + }) +}) + +describe('process', () => { + it('no-relief: a vessel with process lines and no relief device', () => { + const tank = node({ symbolId: 'vessel.tank', kind: 'equipment', tag: { letters: 'TK', loop: '100' } }) + const pump = node({ symbolId: 'pump.centrifugal', kind: 'equipment' }) + const pipe: PlantEdge = { + id: 'e1', lineClass: 'process.major', + source: { nodeId: tank.id, portId: 's' }, target: { nodeId: pump.id, portId: 'suction' }, + } + expect(ids(docOf([tank, pump], [pipe]))).toContain('no-relief') + }) +}) + +describe('data', () => { + it('orphan-record: a record with nothing wearing its key, with a purge fix', () => { + const d = docOf([]) + d.registry = { 'FT-101': { key: 'FT-101', kind: 'instrument', fields: { 'signal.range': 'x' } } } + const group = runRules(buildIndex(d)).groups.find((g) => g.rule.id === 'orphan-record') + expect(group?.findings[0]!.fix?.label).toMatch(/discard/i) + }) + + it('orphan-record: silent while something still wears the key', () => { + const d = docOf([bubble({ letters: 'FT', loop: '101' })]) + d.registry = { 'FT-101': { key: 'FT-101', kind: 'instrument', fields: {} } } + expect(ids(d)).not.toContain('orphan-record') + }) + + it('required-field-empty: silent on a record nobody has started', () => { + // Firing on every tagged object of a pre-registry drawing buried the report + // under 11-13 identical warnings on the bundled samples. + const d = docOf([bubble({ letters: 'FT', loop: '101' })]) + expect(ids(d)).not.toContain('required-field-empty') + }) + + it('required-field-empty: fires once a record is started but incomplete', () => { + const d = docOf([bubble({ letters: 'FT', loop: '101' })]) + d.registry = { 'FT-101': { key: 'FT-101', kind: 'instrument', fields: { 'general.service': 'Feed' } } } + expect(ids(d)).toContain('required-field-empty') + }) + + it('required-field-empty: clears once the required fields are filled', () => { + const d = docOf([bubble({ letters: 'FT', loop: '101' })]) + d.registry = { + 'FT-101': { key: 'FT-101', kind: 'instrument', fields: { 'general.service': 'Feed', 'signal.range': '0-100' } }, + } + expect(ids(d)).not.toContain('required-field-empty') + }) +}) + +describe('typical loops', () => { + // A typical is what the app itself places. If the rule set calls the app's + // own output critical, the rule set is wrong, not the typical. + it('every typical the app places is critical-clean', () => { + for (const t of TYPICALS) { + const doc = createEmptyDoc('t') + const built = buildTypical(t.id, doc, { x: 200, y: 200 }) + doc.sheets[0]!.nodes = built.nodes + doc.sheets[0]!.edges = built.edges + const critical = runRules(buildIndex(doc)).groups + .filter((g) => g.rule.severity === 'critical') + .map((g) => g.rule.id) + expect(critical, `${t.id} raised ${critical.join(', ')}`).toEqual([]) + } + }) +}) diff --git a/tests/validate/suggest.test.ts b/tests/validate/suggest.test.ts deleted file mode 100644 index f6473e4..0000000 --- a/tests/validate/suggest.test.ts +++ /dev/null @@ -1,113 +0,0 @@ -import { describe, expect, it } from 'vitest' -import '../../src/symbols/lib/index' -import { runSuggestions } from '../../src/validate/suggest' -import { buildTypical } from '../../src/assist/typicals' -import { createEmptyDoc } from '../../src/model/doc' -import type { PlantEdge, PlantNode, ProjectDoc, Tag } from '../../src/model/types' - -let n = 0 -const node = (partial: Partial & Pick): PlantNode => ({ - id: `n${n++}`, x: 0, y: 0, rotation: 0, ...partial, -}) -const bubble = (tag: Tag, extra?: Partial): PlantNode => - node({ symbolId: 'instr.bubble', kind: 'instrument', tag, ...extra }) - -function docOf(nodes: PlantNode[], edges: PlantEdge[] = []): ProjectDoc { - const doc = createEmptyDoc('t') - doc.sheets[0]!.nodes = nodes - doc.sheets[0]!.edges = edges - return doc -} -const ids = (doc: ProjectDoc) => runSuggestions(doc).map((s) => s.checkId) - -describe('advisor rules', () => { - it('flags a transmitter nobody receives, clears when a receiver exists', () => { - const lt = bubble({ letters: 'LT', loop: '100' }) - const lic = bubble({ letters: 'LIC', loop: '100' }) - const wire: PlantEdge = { - id: 'e1', lineClass: 'signal.electric', - source: { nodeId: lt.id, portId: 'n' }, target: { nodeId: lic.id, portId: 's' }, - } - expect(ids(docOf([lt], []))).toContain('no-receiver') - expect(ids(docOf([lt, lic], [wire]))).not.toContain('no-receiver') - }) - - it('flags a controller with no final element, clears when its valve exists', () => { - const lic = bubble({ letters: 'LIC', loop: '101' }) - expect(ids(docOf([lic]))).toContain('no-final-element') - const lv = node({ symbolId: 'cv.globe', kind: 'valve', tag: { letters: 'LV', loop: '101' } }) - expect(ids(docOf([lic, lv]))).not.toContain('no-final-element') - }) - - it('flags a tagged instrument with no connections', () => { - expect(ids(docOf([bubble({ letters: 'PT', loop: '100' })]))).toContain('dead-end-instrument') - }) - - it('offers the I/P fix for electric line into a diaphragm valve', () => { - const fic = bubble({ letters: 'FIC', loop: '100' }) - const fv = node({ - symbolId: 'cv.globe', kind: 'valve', x: 0, y: 200, - config: { actuator: 'diaphragm', fail: 'fc' }, tag: { letters: 'FV', loop: '100' }, - }) - const wire: PlantEdge = { - id: 'e1', lineClass: 'signal.electric', - source: { nodeId: fic.id, portId: 's' }, target: { nodeId: fv.id, portId: 'sig' }, - } - const suggestions = runSuggestions(docOf([fic, fv], [wire])) - const hit = suggestions.find((s) => s.checkId === 'needs-ip-converter') - expect(hit).toBeDefined() - // FixSpec is a union now, so narrow before reading the insert-ip payload - const fix = hit!.fix! - expect(fix.kind).toBe('insert-ip') - if (fix.kind !== 'insert-ip') throw new Error('expected an insert-ip fix') - expect(fix.edgeId).toBe('e1') - }) - - it('does not ask for an I/P on solenoid valves or pneumatic lines', () => { - const hs = bubble({ letters: 'HS', loop: '100' }) - const xv = node({ symbolId: 'cv.ball', kind: 'valve', config: { actuator: 'solenoid', fail: 'fc' }, tag: { letters: 'XV', loop: '100' } }) - const wire: PlantEdge = { - id: 'e1', lineClass: 'signal.electric', - source: { nodeId: hs.id, portId: 's' }, target: { nodeId: xv.id, portId: 'sig' }, - } - expect(ids(docOf([hs, xv], [wire]))).not.toContain('needs-ip-converter') - }) - - it('nudges about vessels with process lines but no relief device', () => { - const vessel = node({ symbolId: 'vessel.vertical', kind: 'equipment', label: 'V-101' }) - const pump = node({ symbolId: 'pump.centrifugal', kind: 'equipment', x: 300 }) - const pipe: PlantEdge = { - id: 'e1', lineClass: 'process.major', - source: { nodeId: pump.id, portId: 'discharge' }, target: { nodeId: vessel.id, portId: 'w' }, - } - expect(ids(docOf([vessel, pump], [pipe]))).toContain('no-relief') - const psv = node({ symbolId: 'psv', kind: 'valve', x: 100, y: -100 }) - const reliefLine: PlantEdge = { - id: 'e2', lineClass: 'process.major', - source: { nodeId: vessel.id, portId: 'n1' }, target: { nodeId: psv.id, portId: 'in' }, - } - const cleared = ids(docOf([vessel, pump, psv], [pipe, reliefLine])) - expect(cleared).not.toContain('no-relief') - // and an unpiped vessel is left alone - expect(ids(docOf([node({ symbolId: 'vessel.vertical', kind: 'equipment' })]))).not.toContain('no-relief') - }) - - it('flags control valves without a failure position', () => { - const fv = node({ symbolId: 'cv.globe', kind: 'valve', config: { actuator: 'diaphragm', fail: 'none' } }) - expect(ids(docOf([fv]))).toContain('no-fail-position') - const ok = node({ symbolId: 'cv.globe', kind: 'valve', config: { actuator: 'diaphragm', fail: 'fc' } }) - expect(ids(docOf([ok]))).not.toContain('no-fail-position') - }) - - it('flags valve letters on an instrument bubble', () => { - expect(ids(docOf([bubble({ letters: 'FV', loop: '100' })]))).toContain('valve-tag-on-bubble') - }) - - it('a placed typical loop is advice-clean', () => { - const doc = createEmptyDoc('t') - const { nodes, edges } = buildTypical('flow-control', doc, { x: 0, y: 0 }) - doc.sheets[0]!.nodes = nodes - doc.sheets[0]!.edges = edges - expect(runSuggestions(doc)).toEqual([]) - }) -}) From 659ff7d7d0a665334429e98e71ca1a572c88176b Mon Sep 17 00:00:00 2001 From: PraharshNagpure Date: Wed, 2 Sep 2026 13:52:19 +0530 Subject: [PATCH 2/2] feat: magnetic docking connects mid-drag, with shake-to-disconnect MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Docking used to wait for the mouse button: you dropped a symbol on a connection point, let go, then picked it back up to pull it into place. The line is now drawn the moment the two points meet, with the button still down, and the pipe stretches as the drag carries on. - Symbols stand off 24px from the point they dock onto, along the way that port faces. Landing the points on top of each other read as nothing having happened: the symbols butted together and hid the line behind themselves. - A magnet only joins ports that FACE each other. Two ports pointing the same way stood a symbol on the wrong side of the nozzle it had just connected to, with its inlet pointing away. - Shake the symbol mid-drag and the line that drag made is cut, with a red flash where it landed. The symbol stays in hand, won't snap back onto the point just rejected, and letting go doesn't sneak the connection back on. - The whole gesture is ONE undo step, catch and carry together. Fixes a pointer-event ordering bug found while testing: 'pointerup' on window lands BEFORE the compatibility 'mouseup' JointJS listens on, so the global teardown was closing the drag's undo group early and letting a release re-connect a shaken-off pair. Authored by a concurrent session working in the same checkout; committed here so it ships with v0.17.0 rather than sitting uncommitted. Its edits to files this branch also touches (app.css, store.ts, README, CHANGELOG) landed in the preceding commit — see the note there. Co-Authored-By: Claude Opus 5 (1M context) --- src/canvas/autoConnect.ts | 90 +++++++++++++- src/canvas/interactions.ts | 203 ++++++++++++++++++++++++------- src/canvas/shake.ts | 90 ++++++++++++++ tests/canvas/autoConnect.test.ts | 62 +++++++--- tests/canvas/shake.test.ts | 67 ++++++++++ 5 files changed, 450 insertions(+), 62 deletions(-) create mode 100644 src/canvas/shake.ts create mode 100644 tests/canvas/shake.test.ts diff --git a/src/canvas/autoConnect.ts b/src/canvas/autoConnect.ts index cc851da..6164cf9 100644 --- a/src/canvas/autoConnect.ts +++ b/src/canvas/autoConnect.ts @@ -23,6 +23,7 @@ import type { PortKind } from '../symbols/types' import { getSymbol } from '../symbols/registry' import { portWorld } from './alignment' import { compatibleKinds, pickLineClass } from './connectionRules' +import { type Direction, portDirection, rotateDir } from './shapes' /** * How near a port has to come before it docks, in SCREEN px — the distance is @@ -39,6 +40,50 @@ export function dockRadius(scale: number): number { return Math.min(MAX_SHEET_RADIUS, Math.max(MIN_SHEET_RADIUS, DOCK_SCREEN_PX / s)) } +/** + * Gap left between the two connection points when a symbol docks — three grid + * squares of real, visible pipe. Landing the points on top of each other read + * as "nothing happened": the symbols butted together and hid the line behind + * themselves, so there was no way to tell a connection from a near miss. + */ +export const DOCK_STANDOFF = 24 + +const OPPOSITE: Record = { left: 'right', right: 'left', top: 'bottom', bottom: 'top' } + +/** + * A magnet only joins ports that FACE each other, so the pipe leaves one + * head-on and arrives head-on at the other. Two ports pointing the same way + * would stand the symbol on the wrong side of the one it docked onto — its + * inlet pointing away from the nozzle it just connected to. Ports with no + * catalog direction (user-added pins) put no constraint on the pairing. + */ +function facing(moving: PlantNode, movingPortId: string, target: PlantNode, targetPortId: string): boolean { + const a = portDirection(moving.symbolId, movingPortId) + const b = portDirection(target.symbolId, targetPortId) + if (!a || !b) return true + return rotateDir(a, moving.rotation) === OPPOSITE[rotateDir(b, target.rotation)] +} + +/** Which way the pipe leaves the port that was landed on. */ +function standoff(target: PlantNode, portId: string, approach: { x: number; y: number }, to: { x: number; y: number }): { x: number; y: number } { + const dir = portDirection(target.symbolId, portId) + if (dir) { + switch (rotateDir(dir, target.rotation)) { + case 'left': return { x: -DOCK_STANDOFF, y: 0 } + case 'right': return { x: DOCK_STANDOFF, y: 0 } + case 'top': return { x: 0, y: -DOCK_STANDOFF } + case 'bottom': return { x: 0, y: DOCK_STANDOFF } + } + } + // A user-added pin has no catalog direction: stand off on the side the + // symbol arrived from, so it never jumps across to the far side. + const dx = approach.x - to.x + const dy = approach.y - to.y + return Math.abs(dx) >= Math.abs(dy) + ? { x: dx >= 0 ? DOCK_STANDOFF : -DOCK_STANDOFF, y: 0 } + : { x: 0, y: dy >= 0 ? DOCK_STANDOFF : -DOCK_STANDOFF } +} + export interface Dock { /** Port on the symbol being moved. */ movingPortId: string @@ -47,11 +92,20 @@ export interface Dock { /** Where the moving symbol has to sit for the two ports to coincide. */ x: number y: number - /** Sheet point the ports meet at — where the hint ring is drawn. */ + /** The port that was landed on — where the hint ring is drawn and where + * the line ends. */ at: { x: number; y: number } + /** Where the moving symbol's own port sits once docked: one standoff away + * from `at`, so a real length of pipe shows between them. */ + portAt: { x: number; y: number } lineClass: LineClass } +/** Identifies a port pairing, for refusing one the user has shaken off. */ +export function dockKey(dock: Pick): string { + return `${dock.movingPortId}|${dock.targetNodeId}/${dock.targetPortId}` +} + interface PortRef { nodeId: string portId: string @@ -89,6 +143,7 @@ export function findDock( edges: PlantEdge[], activeLineClass: LineClass, radius: number, + refuse?: ReadonlySet, ): Dock | null { const mine = portsOf(moving) if (!mine.length) return null @@ -105,8 +160,10 @@ export function findDock( // Resolve every candidate port once, not once per port of the moving symbol. const targets: PortRef[] = [] + const byId = new Map() for (const other of others) { if (other.id === moving.id) continue + byId.set(other.id, other) for (const p of portsOf(other)) { const at = portWorld(other, p.id) if (at) targets.push({ nodeId: other.id, portId: p.id, kind: p.kind, x: at.x, y: at.y }) @@ -123,14 +180,22 @@ export function findDock( if (d > bestDistance) continue if (!compatibleKinds(mp.kind, t.kind)) continue if (joined.has(`${moving.id}/${mp.id}|${t.nodeId}/${t.portId}`)) continue + const key = `${mp.id}|${t.nodeId}/${t.portId}` + if (refuse?.has(key)) continue + const other = byId.get(t.nodeId) + if (!other) continue + if (!facing(moving, mp.id, other, t.portId)) continue + const off = standoff(other, t.portId, from, t) + const portAt = { x: t.x + off.x, y: t.y + off.y } bestDistance = d best = { movingPortId: mp.id, targetNodeId: t.nodeId, targetPortId: t.portId, - x: Math.round(moving.x + t.x - from.x), - y: Math.round(moving.y + t.y - from.y), + x: Math.round(moving.x + portAt.x - from.x), + y: Math.round(moving.y + portAt.y - from.y), at: { x: t.x, y: t.y }, + portAt, lineClass: pickLineClass(mp.kind, t.kind, activeLineClass), } } @@ -172,3 +237,22 @@ export function showDockHint(paper: dia.Paper, at: { x: number; y: number } | nu ring.setAttribute('pointer-events', 'none') if (!existing) layer.appendChild(ring) } + +const CUT_CLASS = 'pid-dock-cut' +const CUT_MS = 450 + +/** Red flash where a shaken-off line used to land: the connection is gone, + * and the symbol is still in hand to try somewhere else. */ +export function flashDockCut(paper: dia.Paper, at: { x: number; y: number }): void { + const layer = paper.svg.querySelector('.joint-layers') + if (!layer) return + layer.querySelector(`.${CUT_CLASS}`)?.remove() + const mark = document.createElementNS(NS, 'circle') + mark.setAttribute('class', CUT_CLASS) + mark.setAttribute('r', '10') + mark.setAttribute('cx', String(at.x)) + mark.setAttribute('cy', String(at.y)) + mark.setAttribute('pointer-events', 'none') + layer.appendChild(mark) + window.setTimeout(() => mark.remove(), CUT_MS) +} diff --git a/src/canvas/interactions.ts b/src/canvas/interactions.ts index a67ccdf..bbce768 100644 --- a/src/canvas/interactions.ts +++ b/src/canvas/interactions.ts @@ -10,14 +10,18 @@ import { isPortEnd } from '../model/types' import type { PortKind } from '../symbols/types' import { compatibleKinds, pickLineClass } from './connectionRules' import { alignNodes, distributeNodes, localPortPoint, portWorld, snapGuides } from './alignment' -import { dockEdge, dockRadius, findDock, showDockHint } from './autoConnect' +import { type Dock, dockEdge, dockKey, dockRadius, findDock, flashDockCut, showDockHint } from './autoConnect' +import { createShakeDetector } from './shake' import { cleanVertices } from './vertexClean' import { makeLink } from './shapes' import { getSymbol } from '../symbols/registry' -import { activeSheet, resumeHistory, useStore } from '../store/store' +import { activeSheet, pauseHistory, resumeHistory, useStore } from '../store/store' const snap8 = (v: number) => Math.round(v / 8) * 8 +/** Quiet spell after a shake, so the tail of the waggle docks nothing. */ +const SHAKE_COOLOFF_MS = 600 + type PortKinds = Record function kindFromCell(cell: dia.Cell | undefined, portId: string | null | undefined): PortKind | null { @@ -261,8 +265,13 @@ export function attachInteractions(paper: dia.Paper, graph: dia.Graph): () => vo const onMagnetDown = () => paper.el.classList.add('pid-linking') const onGlobalPointerUp = () => { paper.el.classList.remove('pid-linking') - paper.el.classList.remove('pid-docking') - showDockHint(paper, null) + // A symbol drag is NOT over yet: this 'pointerup' lands before the + // compatibility 'mouseup' JointJS listens on, so element:pointerup — which + // commits the move and closes the docking gesture's undo group — still has + // to run. Tearing down here would cut that group short and let a shaken-off + // connection sneak back on at release. + if (dragStart.size) return + endDockGesture() // safety net: any grouped edit (typing, label drag) ends by now resumeHistory() } @@ -352,19 +361,64 @@ export function attachInteractions(paper: dia.Paper, graph: dia.Graph): () => vo x: hit.x !== undefined ? Math.round(hit.x) : snap8(p.x), y: hit.y !== undefined ? Math.round(hit.y) : snap8(p.y), }) - /** What the dragged node would dock onto at its landing position. */ - const dockAt = (node: PlantNode, hit: ReturnType, p: { x: number; y: number }) => { - const sheet = activeSheet(store()) - return findDock( - { ...node, ...landing(hit, p) }, - sheet.nodes, - sheet.edges, - store().activeLineClass, - dockRadius(paper.scale().sx), - ) + + /** + * Magnetic docking, live inside the drag. + * + * The connection is made the moment the connection points meet — not when + * the mouse button comes up. The symbol clicks into place a standoff away + * so a real length of pipe is visible, the line is written to the document + * there and then, and the user goes on dragging: the pipe stretches behind + * them. Getting it wrong costs nothing — shake the symbol and the line this + * drag made is cut, without ever letting go. + * + * `live` is the connection this gesture made, `holding` whether the magnet + * still has the symbol, and `refused` the pairings shaken off already (so + * the symbol doesn't snap straight back onto the point just rejected). + */ + let live: { edgeId: string; dock: Dock } | null = null + let holding = false + /** A shake happened this drag: the release must not sneak a line back on. */ + let cut = false + /** ...and nothing docks for a moment either, or the tail of the waggle + * catches whatever the symbol was flung past. */ + let dockAgainAt = 0 + const refused = new Set() + const shake = createShakeDetector() + + const endDockGesture = () => { + live = null + holding = false + cut = false + dockAgainAt = 0 + refused.clear() + shake.reset() + showDockHint(paper, null) + paper.el.classList.remove('pid-docking') + } + + /** Is the magnet still close enough to keep the symbol clicked into place? */ + const stillHeld = (node: PlantNode, at: { x: number; y: number }, dock: Dock): boolean => { + const port = portWorld({ ...node, ...at }, dock.movingPortId) + if (!port) return false + return Math.hypot(port.x - dock.portAt.x, port.y - dock.portAt.y) <= dockRadius(paper.scale().sx) } - const onElementPointerMove = (view: dia.ElementView) => { + /** Cut the line this drag docked and let the user aim somewhere else. */ + const cutLive = () => { + if (!live) return + refused.add(dockKey(live.dock)) + flashDockCut(paper, live.dock.at) + store().deleteIds([live.edgeId]) + live = null + holding = false + cut = true + dockAgainAt = Date.now() + SHAKE_COOLOFF_MS + shake.reset() + showDockHint(paper, null) + } + + const onElementPointerMove = (view: dia.ElementView, evt: dia.Event) => { const id = String(view.model.id) const start = dragStart.get(id) if (!start) return @@ -390,15 +444,55 @@ export function attachInteractions(paper: dia.Paper, graph: dia.Graph): () => vo const hit = snapGuides({ ...node, x: p.x, y: p.y }, sheet.nodes, 4, sheet.edges) if (hit.guideX !== undefined) drawGuide(true, hit.guideX) if (hit.guideY !== undefined) drawGuide(false, hit.guideY) - // Magnetic docking preview. Connection dots come up across the sheet so - // the user can see what there is to touch, and the ring marks the point - // this symbol will click onto if they let go now. + + // Docking is a single-symbol gesture: a group drag is being arranged, not + // plumbed, and there is no one symbol whose ports would do the docking. + if (dragStart.size > 1) return + // Connection dots come up across the sheet so the user can see what there + // is to touch. paper.el.classList.add('pid-docking') - const dock = dragStart.size > 1 ? null : dockAt(node, hit, p) + + const pointer = (evt.originalEvent ?? evt) as { clientX?: number; clientY?: number } + if (live && shake.push(pointer.clientX ?? p.x, pointer.clientY ?? p.y, Date.now())) { + cutLive() + return + } + + const at = landing(hit, p) + if (live) { + // Already connected this drag. Hold the symbol on the standoff while + // the pointer stays in reach, then let it go and stretch the pipe. + holding = stillHeld(node, at, live.dock) + if (holding) view.model.position(live.dock.x, live.dock.y) + showDockHint(paper, holding ? live.dock.at : null) + return + } + + if (Date.now() < dockAgainAt) return + const dock = findDock( + { ...node, ...at }, + sheet.nodes, + sheet.edges, + store().activeLineClass, + dockRadius(paper.scale().sx), + refused, + ) showDockHint(paper, dock?.at ?? null) + if (!dock) { + holding = false + return + } + // Caught. Draw the line now, mid-drag — the rest of this gesture is the + // same undo step, so one Ctrl+Z takes the move and the line back together. + live = { edgeId: store().dockNode(id, dock.x, dock.y, dockEdge(id, dock)), dock } + pauseHistory() + holding = true + view.model.position(dock.x, dock.y) } + const onElementPointerDownPos = (view: dia.ElementView) => { dragStart.clear() + endDockGesture() const id = String(view.model.id) const sel = store().selection const ids = sel.includes(id) && sel.length > 1 ? sel : [id] @@ -419,42 +513,61 @@ export function attachInteractions(paper: dia.Paper, graph: dia.Graph): () => vo } } } + const onElementPointerUp = (view: dia.ElementView) => { clearGuides() - showDockHint(paper, null) - paper.el.classList.remove('pid-docking') const id = String(view.model.id) const start = dragStart.get(id) const multi = dragStart.size > 1 - dragStart.delete(id) - if (!start) return + // Emptied here, on every path out: onGlobalPointerUp reads it to tell a + // live drag from a finished one, so a leftover entry would latch the + // undo group open for good. + dragStart.clear() + dragStartVerts.clear() + const docked = live + const wasHolding = holding + const wasCut = cut + endDockGesture() + // The docking burst opened with dockNode and stays one undo step until + // here, whatever the drag did afterwards. + if (!start) return resumeHistory() const p = view.model.position() const sheet = activeSheet(store()) const node = sheet.nodes.find((n) => n.id === id) const hit = node ? snapGuides({ ...node, x: p.x, y: p.y }, sheet.nodes, 4, sheet.edges) : {} - const { x: nx, y: ny } = landing(hit, p) - // Touched a connection point on the way down: click onto it and draw the - // line. Checked before the "didn't move" exit so a symbol nudged back to - // where it started can still dock. - const dock = node && !multi ? dockAt(node, hit, p) : null - if (dock) { - store().dockNode(id, dock.x, dock.y, dockEdge(id, dock)) - dragStart.clear() - dragStartVerts.clear() - return + // A magnet still holding at release keeps the spot it clicked into. + const { x: nx, y: ny } = + docked && wasHolding ? { x: docked.dock.x, y: docked.dock.y } : landing(hit, p) + + // Fallback for a drag that never reported a move inside the reach — a + // flick, or a nudge back onto a point the symbol started next to. Never + // after a shake: the user has just said no to a connection, and letting + // go is not them changing their mind. + if (!docked && !wasCut && node && !multi) { + const dock = findDock( + { ...node, x: nx, y: ny }, + sheet.nodes, + sheet.edges, + store().activeLineClass, + dockRadius(paper.scale().sx), + ) + if (dock) { + store().dockNode(id, dock.x, dock.y, dockEdge(id, dock)) + return resumeHistory() + } } - if (nx === start.x && ny === start.y) return - const dx = nx - start.x - const dy = ny - start.y - const sel = store().selection - if (sel.includes(id) && sel.length > 1) { - const nodeIds = sel.filter((s) => activeSheet(store()).nodes.some((n) => n.id === s)) - store().moveNodes(nodeIds, dx, dy) - } else { - store().setNodePos(id, nx, ny) + if (nx !== start.x || ny !== start.y) { + const dx = nx - start.x + const dy = ny - start.y + const sel = store().selection + if (sel.includes(id) && sel.length > 1) { + const nodeIds = sel.filter((s) => activeSheet(store()).nodes.some((n) => n.id === s)) + store().moveNodes(nodeIds, dx, dy) + } else { + store().setNodePos(id, nx, ny) + } } - dragStart.clear() - dragStartVerts.clear() + resumeHistory() } // --- vertex editing on selected links ---------------------------------- @@ -615,7 +728,7 @@ export function attachInteractions(paper: dia.Paper, graph: dia.Graph): () => vo return () => { clearGuides() - showDockHint(paper, null) + endDockGesture() unsubSelection() window.removeEventListener('keydown', onKeyDown) window.removeEventListener('pointerup', onGlobalPointerUp) diff --git a/src/canvas/shake.ts b/src/canvas/shake.ts new file mode 100644 index 0000000..39fb677 --- /dev/null +++ b/src/canvas/shake.ts @@ -0,0 +1,90 @@ +// SPDX-License-Identifier: PolyForm-Noncommercial-1.0.0 +// Copyright © 2026 Praharsh Nagpure — IPD Studio. Noncommercial use only; +// commercial use requires a paid license (see COMMERCIAL-LICENSE.md). + +/** + * Shake-to-undo, the gesture every phone has taught people: waggle the thing + * you are dragging and the last thing that happened to it comes off again. + * Here it cuts the line a drag just docked, so a wrong connection is undone + * without letting go of the symbol. + * + * A shake is direction reversals: back, forth, back — each leg long enough to + * be deliberate, all of them inside one short window so a slow change of mind + * while routing a symbol across the sheet never reads as one. + */ + +/** How far the pointer must travel one way before a turn counts as a leg. */ +const LEG_PX = 14 +/** All the reversals have to land inside this window. A real waggle turns + * every 100-150ms; re-aiming a symbol across the sheet is far slower. */ +const WINDOW_MS = 600 +/** Reversals that make a shake. Three means back-forth-back. */ +const REVERSALS = 3 + +interface Axis { + dir: -1 | 0 | 1 + travel: number + at: number + seeded: boolean + flips: number[] +} + +const newAxis = (): Axis => ({ dir: 0, travel: 0, at: 0, seeded: false, flips: [] }) + +function step(a: Axis, v: number, now: number): void { + if (!a.seeded) { + a.seeded = true + a.at = v + return + } + const d = v - a.at + a.at = v + if (Math.abs(d) < 1) return + const s = d > 0 ? 1 : -1 + if (a.dir === 0) { + a.dir = s + a.travel = Math.abs(d) + return + } + if (s === a.dir) { + a.travel += Math.abs(d) + return + } + // Turned around: the leg just finished only counts if it was a real one. + if (a.travel >= LEG_PX) a.flips.push(now) + a.dir = s + a.travel = Math.abs(d) +} + +function shaken(a: Axis, now: number): boolean { + a.flips = a.flips.filter((t) => now - t <= WINDOW_MS) + return a.flips.length >= REVERSALS +} + +export interface ShakeDetector { + /** Feed a pointer sample (screen px). True the moment it reads as a shake. */ + push(x: number, y: number, now: number): boolean + /** Forget the gesture so far — after acting on a shake, or on a new drag. */ + reset(): void +} + +export function createShakeDetector(): ShakeDetector { + let ax = newAxis() + let ay = newAxis() + return { + push(x, y, now) { + step(ax, x, now) + step(ay, y, now) + // Either axis on its own is a shake: people waggle sideways, but a + // symbol pinned against the edge of the sheet gets waggled vertically. + if (shaken(ax, now) || shaken(ay, now)) { + return true + } + return false + }, + reset() { + ax = newAxis() + ay = newAxis() + }, + } +} diff --git a/tests/canvas/autoConnect.test.ts b/tests/canvas/autoConnect.test.ts index abead3c..5ab0fef 100644 --- a/tests/canvas/autoConnect.test.ts +++ b/tests/canvas/autoConnect.test.ts @@ -1,6 +1,6 @@ import { describe, expect, it } from 'vitest' import '../../src/symbols/lib/index' -import { dockEdge, dockRadius, findDock } from '../../src/canvas/autoConnect' +import { DOCK_STANDOFF, dockEdge, dockKey, dockRadius, findDock } from '../../src/canvas/autoConnect' import { portWorld } from '../../src/canvas/alignment' import type { PlantEdge, PlantNode } from '../../src/model/types' @@ -19,8 +19,8 @@ const mk = (symbolId: string, x: number, y: number, kind: PlantNode['kind'] = 'v // instr.bubble is 40x40 with n/e/s/w ports of kind 'both'. describe('findDock', () => { - it('docks the near port pair and returns the position that makes them coincide', () => { - const fixed = mk('valve.gate', 200, 200) // e port at (232, 208) + it('docks the near port pair, standing off so a visible pipe is left between', () => { + const fixed = mk('valve.gate', 200, 200) // e port at (232, 208), facing right const moving = mk('valve.gate', 240, 200) // w port at (240, 208) — 8px away const dock = findDock(moving, [fixed], [], 'process.major', 18) expect(dock).not.toBeNull() @@ -28,10 +28,26 @@ describe('findDock', () => { expect(dock!.targetNodeId).toBe(fixed.id) expect(dock!.targetPortId).toBe('e') expect(dock!.lineClass).toBe('process.major') - // the docked position puts the two connection points on the same spot - const docked = { ...moving, x: dock!.x, y: dock!.y } - expect(portWorld(docked, 'w')).toEqual(portWorld(fixed, 'e')) expect(dock!.at).toEqual({ x: 232, y: 208 }) + // one standoff further along the way the port faces, dead in line with it + expect(dock!.portAt).toEqual({ x: 232 + DOCK_STANDOFF, y: 208 }) + const docked = { ...moving, x: dock!.x, y: dock!.y } + expect(portWorld(docked, 'w')).toEqual(dock!.portAt) + }) + + it('stands off along the way the landed-on port faces', () => { + // cv.globe's signal boss points up, so the instrument lands above it. + const cv = mk('cv.globe', 100, 100) // sig at (132, 100), facing top + const bubble = mk('instr.bubble', 114, 64, 'instrument') // s at (134, 104) + const dock = findDock(bubble, [cv], [], 'process.major', 18) + expect(dock!.portAt).toEqual({ x: 132, y: 100 - DOCK_STANDOFF }) + }) + + it('refuses a pairing the caller has shaken off', () => { + const fixed = mk('valve.gate', 200, 200) + const moving = mk('valve.gate', 240, 200) + const refused = new Set([`w|${fixed.id}/e`]) + expect(findDock(moving, [fixed], [], 'process.major', 18, refused)).toBeNull() }) it('finds nothing when no port is within reach', () => { @@ -73,18 +89,27 @@ describe('findDock', () => { expect(dock!.movingPortId).toBe('s') expect(dock!.targetPortId).toBe('sig') expect(dock!.lineClass).toBe('signal.electric') - expect({ x: dock!.x, y: dock!.y }).toEqual({ x: 112, y: 60 }) + expect({ x: dock!.x, y: dock!.y }).toEqual({ x: 112, y: 60 - DOCK_STANDOFF }) }) - it('honors rotation when locating the ports', () => { - const fixed = mk('valve.gate', 200, 200) // e at (232, 208) - // A quarter-turned gate valve: ports run vertically instead. - const moving: PlantNode = { ...mk('valve.gate', 0, 0), rotation: 90 } - const at = portWorld(moving, 'w')! + it('honors rotation when locating the ports and when judging which way they face', () => { + const fixed = mk('valve.gate', 200, 200) // e at (232, 208), facing right + // Turned end for end, so this valve's e port is the one facing left. + const moving: PlantNode = { ...mk('valve.gate', 0, 0), rotation: 180 } + const at = portWorld(moving, 'e')! const shifted = { ...moving, x: moving.x + (232 - at.x) + 5, y: moving.y + (208 - at.y) + 5 } const dock = findDock(shifted, [fixed], [], 'process.major', 18) - expect(dock!.movingPortId).toBe('w') - expect(portWorld({ ...shifted, x: dock!.x, y: dock!.y }, 'w')).toEqual({ x: 232, y: 208 }) + expect(dock!.movingPortId).toBe('e') + expect(portWorld({ ...shifted, x: dock!.x, y: dock!.y }, 'e')).toEqual({ x: 232 + DOCK_STANDOFF, y: 208 }) + }) + + it('only joins ports that face each other', () => { + const fixed = mk('valve.gate', 200, 200) // e at (232, 208), facing right + // This valve's e port also faces right, so butting it up against the + // other one would stand it on the wrong side of the nozzle. + const moving = mk('valve.gate', 208, 200) // e at (240, 208) — 8px away + const dock = findDock(moving, [fixed], [], 'process.major', 18) + expect(dock?.movingPortId).not.toBe('e') }) it('has nothing to dock for a symbol without ports', () => { @@ -107,6 +132,15 @@ describe('dockEdge', () => { }) }) +describe('dockKey', () => { + it('names a pairing so a shaken-off one can be refused for the rest of the drag', () => { + const fixed = mk('valve.gate', 200, 200) + const moving = mk('valve.gate', 240, 200) + const dock = findDock(moving, [fixed], [], 'process.major', 18)! + expect(dockKey(dock)).toBe(`w|${fixed.id}/e`) + }) +}) + describe('dockRadius', () => { it('keeps the reach constant on screen, clamped at extreme zoom', () => { expect(dockRadius(1)).toBe(18) diff --git a/tests/canvas/shake.test.ts b/tests/canvas/shake.test.ts new file mode 100644 index 0000000..f64ad0f --- /dev/null +++ b/tests/canvas/shake.test.ts @@ -0,0 +1,67 @@ +import { describe, expect, it } from 'vitest' +import { createShakeDetector } from '../../src/canvas/shake' + +/** Feed a run of samples along one axis and report when it first reads as a shake. */ +function feed(points: [number, number][], start = 1_000, gap = 40): boolean { + const d = createShakeDetector() + let t = start + let shook = false + for (const [x, y] of points) { + if (d.push(x, y, t)) shook = true + t += gap + } + return shook +} + +/** A waggle: `legs` reversals of `amp` px along x. */ +const waggle = (legs: number, amp = 40): [number, number][] => { + const pts: [number, number][] = [[0, 0]] + for (let i = 0; i < legs; i++) pts.push([i % 2 === 0 ? amp : 0, 0]) + return pts +} + +describe('createShakeDetector', () => { + it('reads a back-forth-back waggle as a shake', () => { + expect(feed(waggle(5))).toBe(true) + }) + + it('ignores a straight drag across the sheet, however long', () => { + const straight: [number, number][] = Array.from({ length: 40 }, (_, i) => [i * 20, i * 6]) + expect(feed(straight)).toBe(false) + }) + + it('ignores one change of mind', () => { + expect(feed([[0, 0], [200, 0], [40, 0]])).toBe(false) + }) + + it('ignores a slow waggle — re-aiming a symbol is not a shake', () => { + expect(feed(waggle(7), 1_000, 400)).toBe(false) + }) + + it('ignores jitter too small to be deliberate', () => { + expect(feed(waggle(9, 6))).toBe(false) + }) + + it('reads a vertical waggle too', () => { + const pts: [number, number][] = [[0, 0]] + for (let i = 0; i < 5; i++) pts.push([0, i % 2 === 0 ? 40 : 0]) + expect(feed(pts)).toBe(true) + }) + + it('forgets the gesture on reset', () => { + const d = createShakeDetector() + let t = 1_000 + for (const [x, y] of waggle(3)) { + d.push(x, y, t) + t += 40 + } + d.reset() + // two more reversals would have tipped it over without the reset + let shook = false + for (const [x, y] of waggle(2)) { + if (d.push(x, y, t)) shook = true + t += 40 + } + expect(shook).toBe(false) + }) +})