Repository navigation
Fix canvas subject area recolor - #57
Merged
Merged
Conversation
…nges An entity moved into or out of a subject area (via MCP add_subject_area_members, a direct YAML edit, or the picker) did not recolour on the canvas: the disk poll refreshed the model and the side drawer updated, but the card kept its old accent. Creating NEW entities in an area worked, because that grows the node set and forces a full re-layout; changing membership of EXISTING entities only moves data.color on nodes whose id and position are unchanged, and React Flow skipped repainting them. Fix (treat a colour delta as render-affecting, not structural): - EntityNode gets an explicit memo comparator keyed on the fields it actually renders (color, dimmed, highlighted, showTypes, collapseDetail, expanded, entity, the hover/highlight Sets, handlers, selected). A bare memo default-compared the whole data object by reference, which did not reliably repaint the accent. - The derivation's keep-positions fast path already carries the fresh colour; it now relies on that comparator so a membership change recolours WITHOUT reflowing. Plays well with saved layout: positions are untouched on a recolour (verified live — the card recolours in place at its exact translate()), so a committed/manual arrangement is preserved. New-entity placement and the subject-area filter are unchanged. Tests: canvas Vitest for the comparator (recolour re-renders; identical fields bail; dimmed/highlighted/expand/selected/entity/Set deltas re-render). Verified live: an out-of-band add_subject_area_members recolours interventions + events to the area colour within one poll, positions unchanged.
…oses drags Repro: with a subject-area filter active and zoomed in, repositioning entities then accidentally clicking one opened the Inspector; closing it zoomed the viewport out AND reverted the just-moved cards. Cause: the keep-positions fast path in the node derivation was gated on !filtering, so while a filter was active EVERY re-run of the derivation — including a selection change like opening or closing the Inspector — fell through to the full re-layout path, which re-fits the viewport (fitView) and re-lays-out the visible subset (dropping any not-yet-saved manual drag). The !filtering gate exists so ticking a filter re-fits the focused domain, but that is a VIEW change, which already resets layoutedRef to false via the view-load effect — not every re-derivation. Fix: key the fast path on layoutedRef alone (drop !filtering). Every entity is always present in newNodes (hidden ones flagged, never removed), so the id/length checks hold across a filter switch too; layoutedRef is what distinguishes 'view changed' (re-layout + fit) from 'selection changed' (keep positions). A real filter toggle still re-fits because its view-load reset makes layoutedRef false. Verified live: filter active + zoomed, open/close Inspector holds both the zoom and every card position. tsc clean; canvas Vitest 55 passed.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Two canvas fixes — live recolour on membership change + Inspector-close no longer disturbs the view
Both fixes live in the same canvas node-derivation / EntityNode rendering area, so they ship together.
Fix 1 — A card now recolours in place when its subject-area membership changes (7bdf7be)
Bug. Moving an existing entity into (or out of) a subject area — via MCP add_subject_area_members, a direct YAML edit, or the picker — updated the model and the side drawer, but the card on the canvas kept its old accent colour. Creating new entities in an area worked (that grows the node set and forces a full re-layout); changing membership of existing entities only mutated data.color on nodes whose id and position were unchanged, and React Flow skipped repainting them.
Root cause (confirmed by live instrumentation). The model data was correct, React’s node data.color was correct, even the inline DOM style was written — but the painted colour stayed stale. EntityNode was a bare memo (default shallow compare of the whole data object), which didn’t reliably repaint the accent.
Fix. An explicit EntityNode memo comparator keyed on the fields it actually renders (color, dimmed, highlighted, showTypes, collapseDetail, expanded, entity, the hover/highlight Sets, handlers, selected). So a membership change recolours in place, without any reflow. Verified live: an out-of-band membership change recolours the card within one disk-poll, position unchanged.
Fix 2 — Closing the Inspector while filtered no longer zooms out or loses drags (bf2268f)
Bug. With a subject-area filter active and zoomed in, repositioning entities then accidentally clicking one opened the Inspector; closing it zoomed the viewport out AND reverted the just-moved cards.
Root cause. The “keep positions” fast-path in the node derivation was gated on !filtering. So while a filter was active, every re-run of the derivation — including a trivial selection change like opening/closing the Inspector — fell through to the full re-layout path, which re-fits the viewport (fitView) and re-lays-out the visible subset (dropping any drag not yet persisted — the layout auto-save is debounced 800ms). The !filtering gate exists so ticking a filter re-fits the focused domain, but that’s a view change, which already resets layoutedRef to false via the view-load effect — not every re-derivation.
Fix. Key the fast-path on layoutedRef alone (drop !filtering). Every entity is always present in the node array (hidden ones flagged, never removed), so the id/length guards hold across a filter switch too; layoutedRef is what distinguishes “view changed” (re-layout + fit) from “selection changed” (keep positions). A real filter toggle still re-fits because its view-load reset makes layoutedRef false. Verified live: filter active + zoomed, open/close Inspector holds both the zoom and every card position.
Tests & verification
New canvas Vitest for the EntityNode comparator (EntityNode.test.tsx): recolour re-renders; identical fields bail; dimmed / highlighted / expand / selected / entity / Set deltas re-render.
Full suite on the merged tree: Python 1008 passed, 1 skipped, 0 failed; canvas tsc clean, Vitest 55 passed; ruff clean.
The rebuilt canvas bundle is included so mdl serve ships both fixes.
Honest caveats
Fix 2 has no automated regression test. It lives inside a large React-Flow effect that isn’t unit-testable without a full browser harness; I guarded it with a detailed in-code comment and verified the fixed behaviour live. A follow-up could extract the position-keeping decision into a pure, unit-testable helper.
Fix 2’s broken state was hard to reproduce frame-for-frame via automation (it needs filter-active + zoomed + an un-saved drag, and synthetic React-Flow drags are finicky), but the code path is unambiguous and the fixed build was verified to behave correctly.