Clue 689 new dataflow nodes should always be visible by lbondaryk · Pull Request #3008 · concord-consortium/collaborative-learning

lbondaryk · 2026-09-22T14:33:33Z

Grid now anchored at the world origin, ignoring the transform's pan that fit-on-load applies
We use positioning rather than autoscale to avoid conflicts with copying tiles or inflight scaling and panning
We are adding tests as well, including some Cypress tests that will only be run on the server

getNewNodePosition laid its grid out from the world origin, ignoring the area transform's pan entirely and converting only the left margin for zoom. Node positions are world coordinates, so once the canvas had been panned — which fit-on-load does by itself — a block added from the palette was created outside the viewport and the button looked dead. The grid is now anchored to the visible rect (screen = world * k + (x, y)), margins convert to world units at the current zoom, and the number of slots is measured from the container so a block's whole footprint fits rather than assuming a fixed 4x5 that ran off the edge of a small tile. The cascade that keeps a full grid's blocks from landing exactly on top of each other is clamped so it cannot walk back out of view. Drag-drop adds are unaffected: they pass their own position. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

…backs in jsdom Two layers the positioning fix was missing: Cypress (dataflow_tool_spec) exercises a real rendered canvas, which is the only place the question 'is the new block on screen' can actually be asked — .editor-graph-container clips overflow, so a block outside the viewport is genuinely hidden and 'be.visible' is a true assertion rather than arithmetic. One case pans the canvas well off the world origin before adding a block; the other fills a grid and checks every block. Both assert the count too, so a block that was never created is distinguished from one created off screen. Jest covers getContainerDimensions, whose real logic — the .cover lookup and the parent walk for a >300x200 ancestor — the positioning tests stub out wholesale. jsdom does no layout, so sizes are defined explicitly; what is under test is which element the method believes. Authored blind: cypress cannot start in this dev environment, so the two browser cases need a CI regression run to validate. They live in an existing spec rather than a new file so they run in the regression shards without a workflow change. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

codecov · 2026-09-22T14:34:50Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 86.13%. Comparing base (3b065b5) to head (a48422d).
⚠️ Report is 154 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #3008      +/-   ##
==========================================
- Coverage   86.95%   86.13%   -0.82%     
==========================================
  Files        1022     1022              
  Lines       58164    58179      +15     
  Branches    15505    15508       +3     
==========================================
- Hits        50574    50110     -464     
- Misses       7568     8045     +477     
- Partials       22       24       +2

Flag	Coverage Δ
cypress	`?`
cypress-regression	`68.70% <100.00%> (-1.98%)`	⬇️
cypress-smoke	`40.80% <13.04%> (-0.04%)`	⬇️
jest	`59.88% <100.00%> (+0.07%)`	⬆️

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-09-22T14:37:38Z

collaborative-learning Run #20434

Run Properties: Passed #20434 • a48422d4a8: CLUE-689: assert canvas containment, not page visibility, for the full grid

Project	`collaborative-learning`
Branch Review	`CLUE-689-new-dataflow-nodes-should-always-be-visible`
Run status	`Passed #20434`
Run duration	`03m 28s`
Commit	`a48422d4a8: CLUE-689: assert canvas containment, not page visibility, for the full grid`
Committer	`Leslie Bondaryk`
View all properties for this run ↗︎

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

Copilot

Copilot review overview

🟡 Changes recommended

Four moderate issues remain in placement logic and Cypress coverage.

Get a fresh assessment by requesting another Copilot review.

Review effort: Lite
Findings: 1

Open (1)

Fixed node height can place generator nodes below the clipped canvas · New

What changed in this PR

This PR updates dataflow node placement so newly added nodes remain within the visible canvas after pan/zoom.

Changes:

Anchors placement to the current transform and viewport dimensions.
Adds fallback sizing and viewport-aware grid placement.
Adds unit and Cypress coverage.

File	Summary
`src/plugins/dataflow/rete/rete-manager.tsx`	Implements viewport-aware placement. Moderate findings: fallback height may clip generator nodes (3 votes); slot reuse may overlap existing nodes (1 vote); default dimensions are not scaled for zoomed-out views (1 vote).
`src/plugins/dataflow/rete/rete-manager.test.ts`	Adds tests for placement and container sizing.
`cypress/e2e/functional/tile_tests/dataflow_tool_spec.js`	Adds visibility tests. Moderate finding: arrow-key pan events may be ignored because the canvas is not activated (1 vote).

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

lbondaryk · 2026-09-22T14:43:58Z

+    const fits = (extent: number, margin: number, node: number, step: number) =>
+      Math.max(1, Math.floor((extent - margin / k - node) / step) + 1);
+    const columns = fits(viewWidth, kLeftMargin, kDefaultNodeWidth, kColumnWidth);
+    const rows = fits(viewHeight, kTopMargin, kDefaultNodeHeight, kRowHeight);


Confirmed and fixed in f7f9a0b. You're right that kDefaultNodeHeight is a measurement fallback rather than a bound — .node.generator .output is at 141px, so the estimate was already wrong for Waves before anything else is accounted for.

Placement now uses a separate kTallestNodeHeight (200), deliberately an upper bound, for both the row capacity and the vertical clamp. It costs one row of capacity in a short tile and guarantees the block fits. I left kDefaultNodeHeight alone since group-bounds measurement genuinely wants an estimate there, and commented why the two differ. A block whose plot is opened later grows past any constant, so this bounds placement only — not the node's lifetime.

Good catch on the coverage too: the Cypress cases only created Number blocks, the shortest type, so they could not have caught this. The panned case now adds a Waves block, and the full-grid case alternates types so the capacity is held to mixed heights.

Copilot caught that kDefaultNodeHeight (120) is the fallback group-bounds measurement uses when it cannot read a node, not a height every block respects: a Waves block puts its output socket alone at 141px. Laying the grid out against it could drop a newly added Waves block below the clipped canvas even though its top-left passed the check. Placement now uses kTallestNodeHeight, a deliberate upper bound rather than an estimate, for both row capacity and the vertical clamp. It costs a row of grid capacity in a short tile and guarantees the block fits. A block whose plot is opened later grows past any constant, so this bounds placement only. The browser coverage only ever created Number blocks — the shortest type, which could not have caught this. The panned case now adds a Waves block, and the full-grid case alternates types so the row capacity is held to mixed heights. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

emcelroy

Looks good 👍 I'm approving but there are some issues to consider addressing before merging. I've left a few inline comments about most of them.

Claude flagged the below and it is a real issue, although in practice it only seems to occur at a high zoom level and/or with lots of blocks. I would still fix it, though.

rete-manager.tsx:1296 - Once the cascade offset pushes a grid slot's position past the clamp's upper bound, every later pass for that same slot clamps to the same spot. Several new blocks land exactly on top of each other and look like one block. A small change to the cascade offset — e.g. wrapping it against the actual leftover space instead of letting it grow unbounded into the clamp — would spread later blocks across distinct positions before they're forced to repeat.

emcelroy · 2026-09-23T12:40:21Z

+    const clamp = (v: number, lo: number, hi: number) => Math.max(lo, Math.min(v, hi));
+
+    return [
+      clamp(originX + Math.floor(slot / rows) * kColumnWidth + pass * kPageOffset,
+        -x / k, -x / k + viewWidth - kDefaultNodeWidth),
+      clamp(originY + (slot % rows) * kRowHeight + pass * kPageOffset,
+        -y / k, -y / k + viewHeight - kTallestNodeHeight)
+    ];


CLUE already uses lodash's clamp elsewhere, here and here. I think that clamp should work fine here as well, and it'd be better to follow that established pattern rather than defining a second one here.

Switched to lodash's clamp in 703cd9a — you're right that a second local definition was gratuitous when the pattern is already established.

emcelroy · 2026-09-23T12:54:46Z

+    // Fall back to the historical grid's extent when the container has not been laid out yet.
+    const viewWidth = dims ? dims.width / k : kColumnWidth * 4;
+    const viewHeight = dims ? dims.height / k : kRowHeight * 5;


Claude initially flagged the issue with lines 1276-1277 described below as important, but further investigation revealed it's more of a nitpick. I would make the suggested change anyway.

For consistency, the fallbacks should divide by k (zoom) like the measured path does. Right now it's the one place the two branches diverge. Not reachable through normal use today (the container is set synchronously in the constructor, so getContainerDimensions() never returns null once the palette button is clickable). If that ever changes, though, this branch could place nodes off-screen again.

Fixed in 703cd9a. Both branches now divide by k: the fallback is expressed as a screen-space extent (kFallbackViewWidth/Height) and converted the same way the measured path is. Agreed it isn't reachable today, but the asymmetry was exactly the sort that turns into an off-screen block the moment the container's lifecycle changes.

emcelroy · 2026-09-23T13:23:17Z

    dataflowToolTile.verifyRecordButtonIcon();
  });
+
+  // CLUE-689. A block created outside the viewport still exists, so the only symptom is that the


I'd remove this Jira story code. Referencing the story doesn't seem necessary and the rest of the comment describes the issue well enough.

Removed in 703cd9a.

emcelroy · 2026-09-23T14:27:11Z

+  it("steps each new block to its own slot rather than stacking them", () => {
+    const transform = { k: 1, x: 0, y: 0 };
+    const first = makePositionStub(0, transform, container).getNewNodePosition();
+    const second = makePositionStub(1, transform, container).getNewNodePosition();
+    expect(second).not.toEqual(first);
+  });


If you you fix the potential block positioning/overlap issue, it'd be good to have this test add many more blocks. With just two it won't catch the problem.

Done in 703cd9a, and you were right that two blocks proved nothing. The new case adds sixteen at k=2, where the visible world is small enough that the grid fills in four and the cascade has only a few units of room — the regime where the unbounded offset actually collides. I mutation-checked it: with the old cascade restored it reports fourteen distinct positions out of sixteen, so it genuinely catches the stacking rather than just exercising more blocks.

…apped cascade - Use lodash's clamp rather than defining a second local one, matching document-scroller and document-browser-scroller. - Divide the fallback view extent by k like the measured path does, so the two branches agree. Not reachable today (the container is set synchronously in the constructor), but the divergence is the kind that places blocks off screen if that ever changes. - Wrap the cascade offset within the room a slot actually has left. Unbounded, it grew until the clamp pinned every later pass to the same spot, so several new blocks landed exactly on top of each other and read as one block. - Drop the story code from the cypress comment; the comment carries the meaning. The stacking test now adds sixteen blocks at a zoom level where the grid fills in four, which is the regime where the cascade has real pressure on it. With the unbounded cascade restored it reports fourteen distinct positions out of sixteen; the two-block version could not have caught that. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

lbondaryk · 2026-09-23T15:43:24Z

Thanks for the review — all five addressed in 703cd9a.

The stacking one you flagged in the review body was the substantive one, and your diagnosis was exactly right: pass * kPageOffset grew without bound, so once a slot's cascade passed the clamp's upper edge, every later pass pinned to the same spot. The offset now wraps within the room each slot actually has left ((pass * kPageOffset) % slack), which spreads later passes across distinct positions instead of repeating one.

Worth noting it only collides where the slack is small, which is why it showed up at high zoom and high block counts as you said — at an unzoomed 800x600 the horizontal slack is ~584px, so it would take hundreds of blocks. The new test pins it at k=2 where the grid fills in four blocks; mutation-checked at 14 distinct positions out of 16 with the old cascade restored.

232 jest tests, check:types clean, lint:build clean. The two Cypress cases still need a regression run to validate — I can't start Cypress in my environment.

…l grid The full-grid case failed in CI on a block covered by the sticky document title after Cypress scrolled to the tile. The block was placed correctly — inside the canvas — but be.visible also requires that nothing else on the page overlaps it, which is not something placement controls, and a full grid reaches the top row where the two collide. It now asserts each block's rect lies within .editor-graph-container, the element that clips the canvas. That is the invariant the code actually guarantees, and a block left at the world origin while the canvas is panned still fails it. The single-block panned case keeps be.visible, which passed and is the stronger assertion where page chrome is not in play. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

lbondaryk and others added 2 commits September 22, 2026 08:58

lbondaryk requested a lite review from Copilot September 22, 2026 14:33

lbondaryk added the run regression label Sep 22, 2026

Copilot started reviewing on behalf of lbondaryk September 22, 2026 14:34 View session

github-actions Bot temporarily deployed to development September 22, 2026 14:37 Inactive

Copilot AI reviewed Sep 22, 2026

View reviewed changes

github-actions Bot temporarily deployed to development September 22, 2026 14:50 Inactive

lbondaryk requested a review from emcelroy September 22, 2026 23:03

emcelroy approved these changes Sep 23, 2026

View reviewed changes

github-actions Bot temporarily deployed to development September 23, 2026 15:49 Inactive

github-actions Bot temporarily deployed to development September 23, 2026 17:07 Inactive

scytacki merged commit 86e5c09 into master Sep 23, 2026
38 of 45 checks passed

scytacki deleted the CLUE-689-new-dataflow-nodes-should-always-be-visible branch September 23, 2026 19:23

Conversation

lbondaryk commented Sep 22, 2026

Uh oh!

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

Uh oh!

Codecov Report

Uh oh!

cypress Bot commented Sep 22, 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

Copilot review overview

🟡 Changes recommended

Uh oh!

Choose a reason for hiding this comment

Uh oh!

emcelroy 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!

lbondaryk commented Sep 23, 2026

Uh oh!

Uh oh!

This branch was previously deployed

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

codecov Bot commented Sep 22, 2026 •

edited

Loading

cypress Bot commented Sep 22, 2026 •

edited

Loading