CLUE-603: highlight Dataflow nodes from a text-tile variable chip by kswenson · Pull Request #2953 · concord-consortium/collaborative-learning

kswenson · 2026-08-10T16:16:37Z

What this delivers

A working, end-to-end highlighting system: one part of a CLUE document can now point at objects inside another tile and make them visually prominent.

Concretely — hover a variable chip in a Text tile and the Dataflow nodes bound to that variable light up; click to pin the highlight, click again to release it.

That user-facing slice is deliberately small, because the point of this PR is that every layer of the mechanism is now built, wired together, and exercised end to end: a reference format, a resolver registry, ephemeral per-user state, a tile-participation contract, and emphasis rendering. It is the minimum viable proof of the whole system, and the precursor to the AI-highlighting work in CLUE-598 — where Ada will emit these same references and the pieces below light up without further plumbing.

Against what CLUE-603 asked for


"Invent a reference system that allows AI to refer to and highlight other tiles and elements within them. Could be based on sparrow targets if convenient."	✅ Delivered, and built on sparrow targets as suggested
"Use variables in a text tile to demonstrate the same connection."	✅ Delivered — this is the demo below
"Use the variables connection between sims and Dataflow as a demonstration. Clicking on the EMG Sensor (should be cast as a chip with a purple chip color…) should highlight any Dataflow node that is associated with the value."	⚠️ Partial — see below

The sims↔Dataflow variables connection is what the demo exercises: the EMG variable originates in the Simulator tile, and the chip highlights the Dataflow node bound to it. What's missing is the Simulator acting as a source — clicking the EMG Sensor itself does nothing yet — and the purple-chip treatment, which is design work. Both are filed as CLUE-621.

Try it

▶ Open the EMG highlight demo

A Text tile reading "Watch this signal drive your program: EMG", a Dataflow program, and the EMG/claw simulator.

Do this	Expect
Hover the EMG chip	The Sensor node gets a dashed ring
Move away	Ring clears
Click the chip	Ring becomes solid and stays
Click again, then move away	Ring drops back to dashed while the pointer is still on the chip, then clears
Delete the chip while pinned	Highlight releases — it can't be stranded

The Live Output node is bound to a different variable and should stay unhighlighted. That's the useful negative check: if everything lights up, the resolver is matching too broadly.

Add your own chips

Nothing here is wired to the one authored chip — the text toolbar's Insert Variable button adds more, and they highlight too:

Insert a Gripper chip and hover it → the Live Output node lights up, not the Sensor. The two chips discriminate correctly, because Gripper is what that node is bound to.
New Variable creates a variable no Dataflow node is bound to, so its chip correctly highlights nothing. That's the mechanism working, not failing.

This is the part worth poking at hardest: the reference system resolves whatever variable a chip names, against whatever the program happens to bind.

What's built

A reference format. HighlightReference is a discriminated union — { kind: "object", tileId, objectId } for a direct pointer, { kind: "variable", variableId } for a semantic query that resolves across tiles. A registry maps each kind to a resolver. Both kinds ship; the object kind is what AI-emitted references will use.

A tile-participation contract. Tiles opt into contributing objects via getObjectsForVariable(variableId), mirroring the existing annotatableObjects hook. Core stays generic — Dataflow's knowledge of how it binds to variables lives in Dataflow.

Ephemeral state. A new DocumentContentModelWithHighlights layer holds the active reference in volatile fields, mirroring how DataSet holds caseSelection for existing table↔graph linked selection. Nothing persists; nothing syncs; nothing is visible to other viewers.

Emphasis rendering. Each tile draws its own emphasis rather than a shared overlay, so highlights inherit the tile's own pan/zoom and clipping for free. Dataflow adds a CSS class to its node.

Builds on the sparrow object-reference system

Sparrows already solved the hard part of addressing — naming an object inside a tile as {tileId, objectId, objectType}, with a tile-side contract (annotatableObjects) that nine tile types already implement. This PR adopts that vocabulary and extends it: the new getObjectsForVariable hook sits alongside annotatableObjects in the same tile-content API, and the object reference kind is that same addressing model.

What's new is everything above the address: resolution of semantic queries into target sets, ephemeral per-user state, and in-tile emphasis. Sparrows' own storage and arrow rendering aren't reused, because a sparrow is persisted document content describing an arrow between two objects, where a highlight is transient emphasis on a set of them.

Coachmarks (@concord-consortium/coachmarks) was also evaluated. It addresses DOM elements by reference or CSS selector, so it doesn't help with the addressing problem — a logical-reference → DOM bridge would still be needed — and its model is tour-shaped rather than "N objects lit at once". Worth revisiting if this grows a guided-tour or anchored-explanation experience.

Implementation notes

Four things a future maintainer would otherwise have to rediscover. All are recorded in docs/highlights.md and in code comments.

Nothing persists. No MST .props() anywhere; all new state is volatile(). A snapshot-invariance test guards it, because adding a prop here would silently publish every student's highlights to everyone.
The resolved-target collection is a closure local, not a .views() getter. MST publishes every view getter as public typed API, and a future text-range kind has no id to express as tileId/objectId. Only isObjectActive/objectState are public.
objectState must be read inside a MobX observer. It's memoized only while a reaction observes it, and it's called once per node per render — hoisting it into a useMemo would make rendering quadratic.
Dataflow nodes read reteManager.tileId rather than walking the MST tree, because rete mounts each node in its own React root, putting TileModelContext and every other CLUE context out of reach.

Testing


`npm test`	335 suites / 3634 tests / 0 failures
`npm run check:types`	0 errors
`npm run lint:build`	0 errors
`highlight_references_spec.js`	3/3, in full regression and standalone

One red check is expected and unrelated: document_tests/tiles_copy_test_spec.js fails in regression. That is not this branch — it reproduces on master (worse there: 4 of its 4 tests fail, versus 1 here). Debugging it turned up a real user-facing regression, now filed as CLUE-622: Copy to Workspace copies no tiles. The count assertion in that spec is a downstream symptom — the workspace is simply empty after the copy. This PR's own spec passes 3/3 in the same runs.

Manually verified in the browser: hover preview, click to pin/unpin, inserting a new variable through the chip toolbar, deleting a chip, and drag-selecting across a chip.

That manual pass earned its keep — it found a bug four rounds of automated review had missed: deleting a chip left its pinned highlight stranded on screen, since clicking the chip is the only way to unpin. Fixed in f9e423a, with a Cypress regression verified to fail against the pre-fix code.

What's next

Each of these is filed and scoped, with the blockers already identified:

Story
CLUE-621	Completes CLUE-603's third bullet — Simulator as a highlight source, with the EMG Sensor recast as a purple chip.
CLUE-598	AI emits references — the payoff. See the comment there for the two concrete blockers: the AI summary strips node ids, and no AI surface renders interactive content today.
CLUE-617	Bidirectional highlighting, and the text tile as a target. Needs no new source — the cheapest route to a second target tile.
CLUE-620	Sketch tile as a target.
CLUE-619	Dataflow wires and groups as targets.
CLUE-618	Highlighting an arbitrary range of text.

The pinned ring's color — currently the same as the Sensor node's own border, so it reads as a thicker border rather than emphasis — is folded into CLUE-621, since that story already involves a design pass on Dataflow chip colors.

Review

A Copilot balanced review of the current head generated no inline comments but four suppressed ones, all considered:

Highlight outline overrode the keyboard focus ring. Real, and fixed in 404b0dc — an element gets only one outline, and .node already spends its on focus-ring(2px), which this file silently overrode by source order. The ring is now a ::after layer; verified in the browser that a node which is both highlighted and focused shows both indicators.
Duplicate chips clobber each other's highlight → filed as CLUE-623. Real but narrow, and the fix is a design change to the state model rather than a local correction.
No keyboard/AT path → filed as CLUE-624.
"Unpinning while hovering doesn't clear the ring" — the behavior is correct (you are still hovering, so a preview ring is right); the PR description was wrong and has been corrected above.

Incidental fixes

Two pre-existing problems found along the way, kept here rather than split out:

manual-regression.yml was unusable for anyone. Both its jobs ran npm ci against the container's node 20.11.0, which no longer satisfies package.json's engines, so the install aborted before any test ran — on any spec, any branch. Added the setup-node step ci-regression.yml already had.
The new spec is listed in that workflow's dispatch menu, so it can be run on its own.

Docs

docs/highlights.md — modeled on the sibling docs/annotations.md: how to make a tile a highlight target or source, the getObjectsForVariable contract, why rendering is in-tile and what should reopen that decision, the three non-obvious preconditions for the variable-chip toolbar, and known limitations.

The design spec and implementation plan this was built from were construction artifacts and are deliberately not kept — everything durable is now in that doc, in code comments, encoded as tests, in the Jira stories above, or here.

_{Note for future pushes: the run regression label is required for CI to run cypress/e2e/functional/**, which is where this feature's only end-to-end coverage lives.}

🤖 Generated with Claude Code

Spec for a reference + ephemeral highlight mechanism so one part of a document can point at objects inside another tile. Key decisions: - Reuse the sparrow *addressing* concept, not its persistence or renderer. Highlights are ephemeral; sparrows are synced document content with no provenance field. - Reject the coachmarks library for v1: it targets DOM elements, so it solves none of the addressing problem, and its model is tour-shaped. - Discriminated-union reference (object | variable) with a resolver registry; Dataflow knowledge stays behind a new optional tile-content hook, getObjectsForVariable. - Volatile state on a new DocumentContentModelWithHighlights layer, following the DataSet volatile-selection precedent. - In-tile rendering (a class on DataflowNode), keeping the state pixel-free so an overlay remains available for CLUE-598. Scope is the mechanism only. AI citation, bidirectional highlighting, and wires/groups as targets belong to CLUE-598.

Leslie confirmed the stories can be reshaped freely, that shortest-path- to-working beats a complete first delivery, and that extending to another tile outranks fleshing out Dataflow's object vocabulary. Changes: - Scope CLUE-603 to increment 1 (skeleton + Dataflow target + variable-chip source); move everything else into a Planned increments section covering bidirectional/text-as-target, AI references, sketch tile, Dataflow wires and groups, and text range highlighting. - Re-test the in-tile rendering decision against three target tiles rather than one. It holds on cost (~35-55 lines across three tiles vs ~150-250 for an overlay) and because in-tile needs no coordinate correction. Record the triggers that should re-open it. - Demote the resolved-target Set to an implementation detail; specify isObjectActive/objectState as the public surface so text ranges (which have no id) can be added later without a breaking refactor. Text range highlighting is recorded honestly rather than assumed cheap: it cannot reuse the void-chip mechanism, and needs either a slate-editor change to expose decorate/renderLeaf or the deferred overlay.

TileReference reads as a reference to a tile. It is not: it is a reference that resolves to a set of highlightable targets. Any Tile* prefix is wrong for half the union -- the variable kind carries no tileId and deliberately resolves across multiple tiles. Object is wrong going forward, since increment 6's textRange kind is not an object in CLUE's vocabulary (ClueObject / annotatableObjects / getObjectBoundingBox all mean a discrete addressable thing with an id). HighlightReference claims nothing false about tile-ness, granularity or multiplicity, matches its module (src/models/highlights/), and pairs with the resolved side: reference in, IHighlightTarget out. Rationale recorded in the spec so it is not re-litigated.

The spec specified the variable chip as the highlight source but never said how a chip gets into a document. Answering that surfaced three preconditions that are all load-bearing and none obvious: - shared-variables-registration is only imported when a Dataflow, Diagram or Simulator tile type is registered; registering Text alone does not pull it in - the unit must list new-variable/insert-variable/edit-variable in settings.text.tools; the app default does not - the buttons are disabled unless a SharedVariables already exists, so the document needs a simulator (or diagram/drawing) tile as the variable source No new UI is needed: InsertVariableTextButton is already a picker over existing SharedVariables, and because the chip binds by id while Dataflow binds by name, both resolve to the same Variable instance. Also records that authored chips round-trip via data-slate-reference and that authored variable ids are stable (simulator-content looks up by name before minting a nanoid), which is what makes a deterministic Cypress fixture possible. demo/docs/chipsimsetup.json is nearly the fixture already -- it just has no chip.

Seven tasks, each with its own test cycle and an independently reviewable deliverable: 1. HighlightReference union + resolver registry + object resolver 2. getObjectsForVariable tile hook + Dataflow implementation 3. variable resolver 4. DocumentContentModelWithHighlights volatile state 5. Dataflow node emphasis rendering (+ getTileIdFromNode) 6. text variable chip as hover/click source 7. EMG demo document + Cypress spec Design decision made while writing it: the tile hook takes a variableId rather than a Variable, so the tile does its own lookup through the sharedVariables accessor it already has. That keeps the resolver free of shared-model plumbing and keeps the derived-string matching inside the plugin that owns it.

Pre-flight scan found two places the plan mandated a test the review rubric would reject: - Task 1's registry-override test left a stub resolver in module-global state for every later test in the file; it now restores in a finally block and asserts the restoration. - Task 6's test passed before the task was implemented, because it only exercised Task 4's actions. The chip handlers are now built by an exported makeChipHighlightHandlers factory so they are testable without a Slate editor, and the test covers the code the task actually adds -- including the two no-op paths. Task 5's after-the-fact test stands: rendering CustomDataflowNode needs a full rete editor and area plugin, and its justification is recorded inline.

…tion

The Task 4 review found the plan's own code could not satisfy the plan's own rule 2. MST publishes every .views() getter as a public instance member, so activeTargetKeys was public typed API on every document -- and the guard test probed 'activeTargets', a name the implementation never used, so it could never fail. Plan now specifies the memoized computed as a closure local inside a single .views() body, returning only isObjectActive/objectState, with the guard test probing the real name. Also folded in: - exported HighlightState type (Task 5 imports it for CSS-class mapping) - a snapshot-invariance test guarding the plan's #1 constraint, which nothing previously covered - activeSource now reports 'pinned' when hovering the already-pinned reference, so a user's own pinned chip does not flicker on hover - documented that the memoization only holds inside a MobX reaction; outside one, every access re-resolves and objectState pays it twice

…nned, snapshot guard)

Manual verification of Slate coexistence (brief Step 5) was not performed in this environment (no browser available); see task-6-report.md.

Adds the demo document (src/public/demo/docs/emg-highlight-demo.json, composed from chipsimsetup.json's structure + old-format-test-document.json's EMG SharedVariables entries verbatim) and the Cypress spec (cypress/e2e/functional/tile_tests/highlight_references_spec.js) proving the CLUE-603 highlight-references feature end to end: hover previews the bound Dataflow Sensor node, click pins it, second click unpins. Adds a second spec case covering Task 6's manual-verification gap (no browser available in that task's subagent): clicking into the text tile and typing past the chip still works and the chip survives the edit. Fixed a bug in that case (cy.type() silently drops keystrokes into Slate's contentEditable since slate-react listens for native beforeinput events, not React synthetic props) by switching to cy.realType/cy.realPress, matching the existing TextToolTile.js helper pattern. No fallback from Task 6's brief was needed — the spec passes against Task 6's primary implementation as committed. Full details in .superpowers/sdd/2026-08-06-clue-603-highlight-references/task-7-report.md.

Applies all seven fixes from the final review of the highlight-references branch: unify the stale four-part document-content chain comments into a single pointer, extract and unit-test the emphasis->CSS class mapping on Dataflow nodes, clear a chip's hover state on unmount (and stop a malformed chip's mouseleave from clobbering another chip's preview), remove the false force:true rationale from the Cypress spec (root-caused the one click that genuinely needs it to the tile's drag-handle icon), correct a comment describing nonexistent code, avoid a per-tick highlight invalidation by using simulatedChannelId instead of the heavier simulatedChannel, and document the intentionally-unused increment-2 scaffolding. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

The preview ring reused $input-purple (#a5b2ff), which is the input nodes' own fill color and measures 2.02:1 against the white .flow-tool canvas -- below the 3:1 minimum WCAG 1.4.11 requires of a non-text state indicator. Replaced with $highlight-preview-ring (#788cff), the nearest accessible neighbor in the same hue family, measured at exactly 3.0:1. The pinned ring is left alone: at 6.42:1 its contrast is fine. It does still equal the Sensor node's own border color, so it reads as a thicker border rather than as emphasis -- recorded as a known limitation in the stylesheet and tracked separately, since choosing an emphasis color that works across every node family is a design decision.

Review note: comments should help a future engineer, not restate library behavior or narrate how the code got here. - document-content-with-highlights.ts: drop the explanation of how MST publishes .views() getters and the reference to what an earlier version of the file did; keep only why the collection must stay encapsulated and the memoization caveat callers actually need. - dataflow-node.tsx: replace the restatement of how MobX observers work with the actionable rule (keep the reads in the render body, and why). - variables-plugin.tsx: drop a rationale sentence duplicated verbatim on the neighboring export. - three test files: remove plan-artifact task numbers and "used to" phrasing; state the invariant and point at the real spec by path. Comments only; no logic changed.

Review note: the conventional way to reach the tile model from a React component is useTileModelContext(). That hook cannot be used here -- rete-react-plugin mounts each node in its own React root (createRoot per node wrapper), so TileModelContext and every other CLUE context resolve to their defaults inside a node. Nothing under plugins/dataflow/nodes/ uses React context for that reason; reteManager is prop-drilled instead. ReteManager already received tileId as a constructor parameter, so make it public and read reteManager.tileId, matching how nodes already get selectNode, announce, and recordedTicks from the same prop. getTileIdFromNode had no other callers, so remove it rather than leave an unused MST helper behind.

Found in manual testing: deleting a variable chip left its pinned highlight on screen permanently. Clicking the chip is the only way to unpin, so once the chip was gone the highlight could not be dismissed for the rest of the session. The unmount cleanup added earlier only cleared hoveredRef. Added releaseOwnHighlightRefs, which releases both the hover and the pin when either belongs to the chip being unmounted, and used it from the unmount effect. Mouse-leave deliberately still clears only the hover -- leaving a pinned chip must keep the pin. Covered by a Cypress case, since proving this needs a real Slate unmount. Verified the case fails against the pre-fix code with exactly the reported symptom (chip gone, highlight-pinned still present). The demo document's paragraph now ends with the chip so End+Backspace targets it unambiguously. Clicking a Slate inline void places the caret beside it rather than selecting it, so the test cannot delete the chip by clicking it, and walking the caret by character count would break the moment the demo prose changed.

codecov · 2026-08-10T16:21:43Z

Codecov Report

❌ Patch coverage is 99.21260% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 86.14%. Comparing base (53d7d05) to head (404b0dc).

Files with missing lines	Patch %	Lines
src/models/highlights/highlight-reference.ts	96.29%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #2953      +/-   ##
==========================================
+ Coverage   86.07%   86.14%   +0.06%     
==========================================
  Files         980      982       +2     
  Lines       55955    56076     +121     
  Branches    14754    14791      +37     
==========================================
+ Hits        48164    48306     +142     
+ Misses       7771     7750      -21     
  Partials       20       20

Flag	Coverage Δ
cypress	`?`
cypress-regression	`71.34% <92.85%> (+0.11%)`	⬆️
cypress-smoke	`41.30% <29.46%> (-0.04%)`	⬇️
jest	`56.93% <90.55%> (+0.20%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

cypress · 2026-08-10T16:23:29Z

collaborative-learning Run #19784

Run Properties: Passed #19784 • 404b0dc762: CLUE-603: draw the highlight ring as a layer so focus stays visible

Project	`collaborative-learning`
Branch Review	`CLUE-603-linked-representation-references`
Run status	`Passed #19784`
Run duration	`10m 40s`
Commit	`404b0dc762: CLUE-603: draw the highlight ring as a layer so focus stays visible`
Committer	`Kirk Swenson`
View all properties for this run ↗︎

Test results
Failures	`0`
Flaky	`1`
Pending	`5`
Skipped	`0`
Passing	`223`
View all changes introduced in this branch ↗︎

Copilot

Pull request overview

Implements increment 1 of CLUE-603’s cross-tile highlight reference system: hovering/clicking a Text tile variable chip drives per-session (volatile) highlighting of the corresponding Dataflow nodes.

Changes:

Added a HighlightReference + resolver registry and a new volatile highlight layer on DocumentContentModel (hovered vs pinned precedence).
Implemented a new optional tile-content hook (getObjectsForVariable) and wired Dataflow + Text variable chips to participate.
Added styling plus Jest and Cypress coverage, and introduced an EMG demo document fixture for deterministic E2E testing.

Reviewed changes

Copilot reviewed 23 out of 23 changed files in this pull request and generated 2 comments.

Show a summary per file

File	Description
src/public/demo/docs/emg-highlight-demo.json	Adds a deterministic demo document/fixture with an authored variable chip and EMG simulation wiring.
src/plugins/shared-variables/slate/variables-plugin.tsx	Wires variable chips to set/clear/toggle document highlight references and cleans up on unmount.
src/plugins/shared-variables/slate/variables-plugin-highlight.test.ts	Unit tests for chip highlight handler behavior and cleanup guards.
src/plugins/dataflow/rete/rete-manager.tsx	Exposes `tileId` publicly for rete-mounted node React roots to access tile context.
src/plugins/dataflow/nodes/node-states.scss	Adds pinned/preview highlight ring styling for nodes.
src/plugins/dataflow/nodes/dataflow-node.tsx	Reads document highlight state and applies highlight classes to Dataflow nodes (via `classNames`).
src/plugins/dataflow/nodes/dataflow-node-highlight.test.ts	Unit tests for emphasis→class mapping and highlight state wiring expectations.
src/plugins/dataflow/model/utilities/simulated-channel.ts	Exports `simulatedChannelId()` for reuse when matching variable bindings.
src/plugins/dataflow/model/dataflow-content.ts	Implements `getObjectsForVariable()` by scanning Sensor/Live Output nodes for variable bindings.
src/plugins/dataflow/model/dataflow-content.test.ts	Adds tests for `getObjectsForVariable()` matching behavior and known derived-string fragility.
src/models/tiles/tile-model-hooks.ts	Introduces `getObjectsForVariable(variableId)` hook with a default no-op implementation.
src/models/highlights/highlight-reference.ts	Adds `HighlightReference` union, target keying, resolver registry, and variable/object resolvers.
src/models/highlights/highlight-reference.test.ts	Tests resolver behavior, registry overriding, and reference equality helpers.
src/models/document/drag-tiles.ts	Updates doc comment to reference the canonical composition explanation in `document-content.ts`.
src/models/document/document-content.ts	Inserts the new highlights composition layer into the document content model chain.
src/models/document/document-content-with-highlights.ts	New composition layer storing hovered/pinned refs in `volatile()` and exposing `objectState()/isObjectActive()`.
src/models/document/document-content-with-highlights.test.ts	Tests precedence rules, toggling, non-exposure of internal target collection, and snapshot invariance.
src/models/document/document-content-with-annotations.ts	Updates doc comment to reference canonical composition explanation.
src/models/document/base-document-content.ts	Updates doc comment to reference canonical composition explanation.
src/components/tiles/text/text-tile.scss	Adds pointer cursor affordance for clickable variable chips.
docs/superpowers/specs/2026-08-04-clue-603-linked-representation-references-design.md	Adds/records design spec for the reference + highlighting architecture and constraints.
docs/superpowers/plans/2026-08-06-clue-603-highlight-references.md	Adds implementation plan documenting constraints, tasks, and testing strategy.
cypress/e2e/functional/tile_tests/highlight_references_spec.js	End-to-end coverage for hover preview, click pin/unpin, editing safety, and chip deletion cleanup.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Copilot review: DocumentContentModelType and HighlightReference in variables-plugin.tsx, and HighlightState in dataflow-node.tsx, are used only in type positions, so a value import needlessly pulls the document-content module graph in at runtime. This also makes the branch internally consistent -- highlight-reference.ts and document-content-with-highlights.ts already import DocumentContentModelType with 'import type' to break a real require cycle.

…pping The test passed locally but failed all three attempts in CI. It pressed End then Backspace, and End goes to the end of the visual line -- CI's viewport and font metrics wrap the demo paragraph differently, so Backspace ate a character of the prose while the chip survived. Select the paragraph and delete instead, which depends on neither wrapping nor caret position, and establish focus the way TextToolTile.enterText does (focus the tile, then click the editor, after a wait for editor accessibility) rather than a bare force-click. Re-verified the restructured test still fails against the pre-fix code with the reported symptom. Also list the new spec in manual-regression.yml so it can be dispatched on its own -- every other spec is listed, and a full regression cycle is a slow way to iterate on one test.

Both jobs in manual-regression.yml ran npm ci against the container image's baked-in node 20.11.0, which no longer satisfies package.json's engines (^20.19.0 || ^22.13.0 || >=24). The install aborted with ERR!notsup before any test ran, so the workflow was unusable regardless of which spec was selected. Added the same actions/setup-node@v4 step ci-regression.yml already uses to install a current node 20.x. Pre-existing breakage, found while trying to run a single spec instead of a full regression cycle.

The spec and plan were construction artifacts and would go stale. Audited both for content not captured elsewhere: Already captured, dropped: - derived-string variable binding and its rename fragility -> a comment in dataflow-content.ts plus a test that encodes the behavior - the volatile/never-persisted rule and the private-target-Set rationale -> doc comments in document-content-with-highlights.ts - the sparrows and coachmarks evaluation -> the PR description - per-task implementation steps -> the code itself Not captured anywhere, so moved into docs/highlights.md (modeled on the sibling docs/annotations.md): - how to make a tile a highlight target or source, and the two rules a source must respect - the getObjectsForVariable contract and why its optional call is load-bearing - why rendering is in-tile rather than an overlay, and the two triggers that should reopen that decision - the three non-obvious preconditions for the variable-chip toolbar, which docs/variables.md alludes to as 'set up properly' without saying how - known limitations The planned increments are deliberately not in the repo -- that is roadmap, and belongs in Jira where it can be reprioritized.

Copilot

Pull request overview

Copilot reviewed 23 out of 23 changed files in this pull request and generated no new comments.

Suppressed comments (4)

src/plugins/shared-variables/slate/variables-plugin.tsx:312

Clicking a pinned chip while the pointer is still over it does not clear the ring as described: hoveredRef remains set, so toggling off pinnedRef immediately downgrades the target to preview until mouse-out. Clear the chip's hover ref when the click is unpinning it.

    onClick: () => {
      if (variableId) documentContent?.togglePinnedRef({ kind: "variable", variableId });
    },

src/plugins/shared-variables/slate/variables-plugin.tsx:347

This cleanup cannot tell which chip instance owns the reference; it only compares variableId. If the same variable is inserted in two chips, deleting or otherwise unmounting either chip clears a pin/preview created by the other chip. Track a per-source owner token with the volatile refs (and clear only when that token matches), so duplicate chips do not cancel one another.

  useEffect(() => {
    return () => releaseOwnHighlightRefs(documentContent, reference);
  }, [documentContent, reference]);

src/plugins/shared-variables/slate/variables-plugin.tsx:383

This span now performs a button-like pin/unpin action only through onClick, but it is not focusable and has no button semantics or keyboard activation. Keyboard and assistive-technology users therefore cannot use the new interaction. Add an appropriate role, focusability, pressed state, and Enter/Space handling without disrupting Slate editing.

    <span
      className={classes}
      {...attributes}
      contentEditable={false}
      onMouseEnter={highlightHandlers.onMouseEnter}
      onMouseLeave={highlightHandlers.onMouseLeave}
      onClick={highlightHandlers.onClick}

src/plugins/dataflow/nodes/node-states.scss:82

These outline declarations override the node's existing keyboard focus ring. dataflow-node.scss:34 applies focus-ring(2px), but node-states.scss is imported afterward, and .node.highlight-preview has the same specificity as .node:focus-visible/.node.keyboard-focused; the later highlight outline wins. A focused highlighted node consequently has no distinguishable focus indicator. Render highlight emphasis with a separate layer (for example a pseudo-element) so focus and highlight can coexist; apply the same correction to the pinned rule below.

.node.highlight-preview {
  outline: 3px dashed $highlight-preview-ring;
  outline-offset: 2px;

Copilot balanced review, suppressed comment: the highlight used outline, but an element gets only one outline and .node already spends its outline on the keyboard focus ring (focus-ring(2px) in dataflow-node.scss). node-states.scss is imported after that file and .node.highlight-preview has the same specificity as .node:focus-visible, so the highlight silently won and a focused highlighted node had no focus indicator -- in a tile with substantial keyboard navigation. Draw the ring as a ::after layer instead, leaving outline free. Verified in the browser that a node which is both highlighted and keyboard-focused now shows the 3px dashed ring AND the 2px solid focus outline. Same constraint the socket rules further up this file already work around.

tealefristoe

I've only started looking at this, but I encountered some odd behavior when trying the demo which made me wonder if this is really delivering what's expected (even if it's delivering what's technically being asked).

Here's the odd behavior:

Open the demo document.
Click on the chip in the text tile to highlight the input node in the dataflow tile.
Click on other text in the text tile.

What happens:

The text chip is no longer selected, but the dataflow node remains highlighted.
If you click the chip again, it becomes selected, and the dataflow node stops being highlighted.

Another thing that seems odd to me is that you can click on the text chip repeatedly to toggle the dataflow node highlight on and off, but the text chip stays selected that whole time.

I suspect that what Leslie really wants is for everything related to a variable to be highlighted whenever anything related to the variable is selected. I think it would be good to check in about this before moving forward in this direction.

I'm also wondering if the long term plan here is to allow a user to highlight things in the same way that an AI would highlight things. I would evaluate things differently if this is a temporary step to prove that things can be highlighted versus a permanent feature.

Finally, I think there will be a need to highlight multiple things at some point, so if we do go in this direction it would be good to make the pinnedRef a set instead of a single item.

tealefristoe · 2026-08-12T19:17:17Z

I think it would be good to split this document into two, one about highlights generally and one about the specific highlights introduced in this PR. If I'm expanding highlights to a new tile or new objects within a tile, I don't really care how to add variable buttons to the text toolbar.

Teale's point is correct. I expect this to work like highlighting between linked data elements. So if you deselect a row in a table it deselects the data points that go with it. If you reselect everything turns back on.

tealefristoe · 2026-08-12T19:22:50Z

+// Type-only import: document-content.ts -> document-content-with-highlights.ts -> this file
+// would close a runtime require cycle if this were a value import. See the equivalent note in
+// highlight-reference.ts. Do not change this to a value import.
+import type { DocumentContentModelType } from "./document-content";


See my comment in highlight-reference.ts about this. Long story short, I don't think you should have to import anything here, or cast self when you call resolveHighlightReference below.

tealefristoe · 2026-08-12T19:32:51Z

+    //
+    // This `computed` only caches while a MobX reaction observes it. Callers should read it from
+    // inside an `observer` — outside one, every `.get()` re-resolves, walking every tile again.


Suggested change

//

// This `computed` only caches while a MobX reaction observes it. Callers should read it from

// inside an `observer` — outside one, every `.get()` re-resolves, walking every tile again.

Given this is basic mobx info and the function can only be used within this views anyway, I don't think the comment is needed.

tealefristoe · 2026-08-12T19:51:18Z

+ * Resolve a reference to its targets. Fails quiet: an unknown kind yields no targets.
+ */
+export function resolveHighlightReference(
+  ref: HighlightReference, content: DocumentContentModelType


It looks like you only ever reference tileMap from content, so you could make this BaseDocumentContentModel instead of DocumentContentModelType. That way you wouldn't have to import DocumentContentModelType in a weird way with a multi-line comment here, and you wouldn't have to cast self (and therefore import a type at all) in document-content-with-highlighting.ts.

tealefristoe · 2026-08-12T19:59:04Z

+    return {
+      isObjectActive(tileId: string, objectId: string) {
+        return activeTargetKeys.get().has(highlightTargetKey(tileId, objectId));
+      },
+      /**
+       * Every active target shares one state, because only one reference is active at a time.
+       * This can never return "pinned" for one object while returning "preview" for another in
+       * the same render.
+       */
+      objectState(tileId: string, objectId: string): HighlightState | undefined {
+        return activeTargetKeys.get().has(highlightTargetKey(tileId, objectId))
+          ? self.activeSource
+          : undefined;
+      },
+    };


Suggested change

return {

isObjectActive(tileId: string, objectId: string) {

return activeTargetKeys.get().has(highlightTargetKey(tileId, objectId));

},

/**

* Every active target shares one state, because only one reference is active at a time.

* This can never return "pinned" for one object while returning "preview" for another in

* the same render.

*/

objectState(tileId: string, objectId: string): HighlightState | undefined {

return activeTargetKeys.get().has(highlightTargetKey(tileId, objectId))

? self.activeSource

: undefined;

},

};

function isObjectActive(tileId: string, objectId: string) {

return activeTargetKeys.get().has(highlightTargetKey(tileId, objectId));

}

return {

isObjectActive,

/**

* Every active target shares one state, because only one reference is active at a time.

* This can never return "pinned" for one object while returning "preview" for another in

* the same render.

*/

objectState(tileId: string, objectId: string): HighlightState | undefined {

return isObjectActive(tileId, objectId) ? self.activeSource : undefined;

},

};

I also think these functions should be renamed to something like isObjectHighlighted and objectHightlightState.

tealefristoe · 2026-08-12T20:10:16Z

+    get activeRef(): HighlightReference | undefined {
+      return self.hoveredRef ?? self.pinnedRef;
+    },
+    get activeSource(): HighlightState | undefined {


I think these should be called something like highlightRef and highlightState. activeRef and activeSource aren't obviously about highlighting, and these are being added to the document model, where these terms could reasonably refer to a number of different things.

tealefristoe · 2026-08-12T20:14:35Z

+    hoveredRef: undefined as HighlightReference | undefined,
+    pinnedRef: undefined as HighlightReference | undefined,


Maybe hoveredRef is ok, because it could be used in non-highlight situations, but pinnedRef should probably be renamed something like pinnedHighlightRef, and its setters should be renamed too.

kswenson and others added 20 commits August 10, 2026 17:29

CLUE-603: add HighlightReference type and resolver registry

b535818

CLUE-603: add getObjectsForVariable tile hook and Dataflow implementa…

95d6b23

…tion

CLUE-603: add the variable reference resolver

1e27ec0

CLUE-603: add volatile highlight state to the document content model

446ad32

CLUE-603: fix Task 4 review findings (private target Set, hover-on-pi…

ebc0d08

…nned, snapshot guard)

CLUE-603: render highlight emphasis on Dataflow nodes

d932b07

CLUE-603: drive highlights from the text variable chip

b952b08

Manual verification of Slate coexistence (brief Step 5) was not performed in this environment (no browser available); see task-6-report.md.

github-actions Bot temporarily deployed to development August 10, 2026 16:22 Inactive

kswenson requested a lite review from Copilot August 10, 2026 17:57

Copilot started reviewing on behalf of kswenson August 10, 2026 17:57 View session

Copilot AI reviewed Aug 10, 2026

View reviewed changes

Comment thread src/plugins/shared-variables/slate/variables-plugin.tsx Outdated

Comment thread src/plugins/dataflow/nodes/dataflow-node.tsx Outdated

kswenson added the run regression label Aug 10, 2026

github-actions Bot temporarily deployed to development August 10, 2026 18:15 Inactive

github-actions Bot temporarily deployed to development August 10, 2026 18:55 Inactive

github-actions Bot temporarily deployed to development August 10, 2026 19:00 Inactive

github-actions Bot temporarily deployed to development August 11, 2026 12:50 Inactive

kswenson requested a balanced review from Copilot August 11, 2026 12:56

Copilot started reviewing on behalf of kswenson August 11, 2026 12:56 View session

Copilot AI reviewed Aug 11, 2026

View reviewed changes

github-actions Bot temporarily deployed to development August 11, 2026 14:12 Inactive

kswenson requested a review from tealefristoe August 11, 2026 15:09

kswenson mentioned this pull request Aug 12, 2026

CLUE-620: make the sketch tile a highlight target #2956

Open

tealefristoe reviewed Aug 12, 2026

View reviewed changes

	//
	// This `computed` only caches while a MobX reaction observes it. Callers should read it from
	// inside an `observer` — outside one, every `.get()` re-resolves, walking every tile again.

		hoveredRef: undefined as HighlightReference \| undefined,
		pinnedRef: undefined as HighlightReference \| undefined,

Uh oh!

Conversation

kswenson commented Aug 10, 2026 • edited by atlassian Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What this delivers

Against what CLUE-603 asked for

Try it

Add your own chips

What's built

Builds on the sparrow object-reference system

Implementation notes

Testing

What's next

Review

Incidental fixes

Docs

Uh oh!

codecov Bot commented Aug 10, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

cypress Bot commented Aug 10, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

tealefristoe left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

kswenson commented Aug 10, 2026 •

edited by atlassian Bot

Loading

codecov Bot commented Aug 10, 2026 •

edited

Loading

cypress Bot commented Aug 10, 2026 •

edited

Loading