diff --git a/packages/pluggableWidgets/tree-node-web/CHANGELOG.md b/packages/pluggableWidgets/tree-node-web/CHANGELOG.md index 62c97f4958..af0a12fce4 100644 --- a/packages/pluggableWidgets/tree-node-web/CHANGELOG.md +++ b/packages/pluggableWidgets/tree-node-web/CHANGELOG.md @@ -6,6 +6,13 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/), ## [Unreleased] +### Fixed + +- We fixed an issue where a tree node's loading spinner would never disappear when using a microflow data source with "Start expanded" set to yes. +- We fixed an issue where expanding one node could permanently remove the expand icon from an unrelated, unexpanded node elsewhere in the tree when using a microflow data source. +- We fixed an issue where a node's expand icon for a deeper tier would not appear until that node was collapsed and expanded again. +- We fixed an issue where, with "Start expanded" set to yes, tree nodes deeper than the second level would not show an expand icon until a parent node was manually collapsed and expanded again. + ## [3.11.0] - 2026-05-27 ### Added diff --git a/packages/pluggableWidgets/tree-node-web/CONTEXT.md b/packages/pluggableWidgets/tree-node-web/CONTEXT.md new file mode 100644 index 0000000000..be4902d1ec --- /dev/null +++ b/packages/pluggableWidgets/tree-node-web/CONTEXT.md @@ -0,0 +1,45 @@ +# Tree Node widget — domain model + +## v1 vs v2 + +`TreeNode.tsx` (root dispatcher) routes by whether `parentAssociation` is configured: + +- **`parentAssociation` set → v2** (`src/components/v2/`): self-referencing "infinite tree" mode. One widget instance renders the whole tree; each item's own association tells the engine its parent. Added in 3.11.0. +- **`parentAssociation` unset → v1** (`src/components/v1/`): manual-nesting mode. Each tree level is a separately-configured widget instance, nested inside the parent level's "children" slot in Studio Pro. + +### `hasChildren` prop is v1-only, architecturally + +`hasChildren: ListExpressionValue` (XML caption "Has children") exists so a Studio Pro developer can declare per-item whether a node has children, when there's no other way to know (v1: no association, no structural signal). `TreeNode.editorConfig.ts:38-39` hides this property from Studio Pro whenever `parentAssociation` is configured — i.e. whenever v2 is in use. It has no XML default value, so on a v2 widget instance `props.hasChildren` is `undefined` at runtime, not just "possibly misconfigured." **v2 must never read `props.hasChildren` — it will crash.** (Confirmed live during WC-3564: `TypeError: Cannot read properties of undefined (reading 'get')`.) + +For v2, "does this node have children" is derived structurally: `node.children.length > 0`, where `node.children` is populated by matching real association values across the full item set the datasource has delivered so far (`useIncrementalTreeData.ts`). `useInfiniteTreeNode.ts`'s preload mechanism (`loadedChildsByIdRef`) fetches one level ahead of every expand specifically so a node's own children are already known by the time that node is rendered as clickable — this is what keeps `node.children.length > 0` from being stale/late in practice. + +There is no cheaper way to know this without fetching. Mendix's pluggable-widget `ListValue` API is one shared, filter-driven list — no per-item "does this have children" or count-only primitive exists for pluggable widgets. Answering "does node N have children" means asking the datasource for items whose parent is N and seeing what comes back; the preload mechanism exists because that's the only way this API surface allows for v2's configuration mode, not because of a missed optimization. + +## `TreeNodeState.LOADING` (v2) — spinner, not a stored per-node fact (WC-3564) + +Originally (pre-WC-3564) a freshly-created node started in `LOADING` and only left it once its own id reappeared in a _later_ datasource delivery — meant to avoid the expand icon "popping in" late for nodes that would turn out to have children. That resolution criterion was wrong: a microflow datasource always redelivers its full flattened result regardless of the filter passed to `setFilter`, so _any_ later delivery could match, not just one that actually answered a given node's question. Two bugs followed: + +- Bug 1: with "Start expanded" = Yes, no later delivery route existed at all for the frozen microflow case — permanent spinner. +- Bug 2: with "Start expanded" = No, expanding any node caused a full redelivery that could wrongly resolve an unrelated, unexpanded node into a childless state — permanently killing its icon. + +Fix: `LOADING` is no longer stored on the node at all. A node is created directly as `EXPANDED`/`COLLAPSED_WITH_JS` (per `startExpanded`) — never `LOADING`. The spinner is a pure render-time decision in `TreeNode.tsx`: show it when a node has no known children yet (`node.children.length === 0`) **and** `props.datasource.status === ValueStatus.Loading` — Mendix's own, real-time, first-party "is this datasource actually fetching right now" signal. No per-node bookkeeping, so nothing can be "stuck" (the flag is never persisted) and nothing about one node's resolution can affect another's (it's a single global flag, not a per-node mutation). + +## `useInfiniteTreeNode.ts`'s one-level-lookahead preload — content-gated, not fire-count-gated + +`appendItems` (called on every click-to-expand) and the bootstrap `useEffect` (auto-expand for `startExpanded = Yes`) both implement the same idea: when a node's children become known, also preload _their_ children's existence one level further ahead, so an already-visible child's own expand affordance is correct without the user needing to click into it first. Both had the same class of bug (WC-3564, found during manual verification, pre-existing on `main`, unrelated to the `LOADING`/`hasChildren` mechanism above): + +- `appendItems` gated the preload step behind "was this node already a loaded-parent" — true only from a node's _second_ expand onward, so the first expand never preloaded its children's children. A collapse+re-expand of the _same_ node was required to see a deeper tier. **Fixed**: removed that gate — the preload now runs unconditionally whenever children are passed in. Verified live. +- The bootstrap effect capped itself at exactly _one_ automatic round ever (`loadedParentsByIdRef.current.size === 0`, true only once) — so `startExpanded = Yes` roots got their own children preloaded, but never one level further. A collapse+re-expand of a _root_ node was required to see a deeper tier. + + **First attempt at a fix broke live testing**: extended the cap to exactly 2 rounds via a plain counter that advanced on every effect firing. Passed unit tests against a mock, but broke the real "Expanded bug" repro project — the tree stopped rendering anything past the root level. Root-caused with temporary per-widget-tagged debug logging (three tree widgets mount simultaneously on that page regardless of active tab, so untagged logs were unreadable): three tree widgets on the page were logging interleaved, and once tagged, the trace showed both rounds fired — and locked themselves in — while `datasource.items` was still transiently empty during initial load, before the real root items ever arrived. A counter can't tell "fired" apart from "fired with something worth preloading." + + **Second attempt**: replaced the counter with two content-based flags (`round1DoneRef`, `round2DoneRef`) that only flip once real, not-yet-tracked items are actually found — mirroring the pre-existing round-1 gate's own self-correcting semantics (checked _after_ attempting to populate, so it harmlessly retries on an empty delivery instead of locking in early). Verified live against a 2-tier "Expanded bug" dataset at the time — worked. Against a deeper (4-tier) dataset, it turned out still insufficient: the 3rd tier showed up as content but without its own icon, needing a real click on an ancestor to reveal — the fixed 2-round cap was itself the bug, just less obviously than the original one-round cap. + + **Final fix**: since every level defaults to `EXPANDED` (not just roots) under `startExpanded = Yes`, replaced the round cap entirely with an unbounded, self-terminating cascade — keep treating newly-arrived items as loaded-parents and fetching their children for as long as new descendants appear, stop once a round finds nothing new (bounded by the tree's real depth, not a count). Scoped specifically to `startExpanded === true` (confirmed by asking — see decisions log): `startExpanded = false` keeps the original capped round1+round2 behavior unchanged, since deeper tiers there stay collapsed by default and already resolve correctly via a single click. Verified live against the real 4-tier dataset: every tier shows its correct expand affordance automatically, no manual toggle needed anywhere. + +**Lesson, still worth keeping in mind**: a change here can look bounded/safe by code inspection and pass every mocked unit test, yet still break against a real datasource, because mocks don't reproduce the transient "still loading, items temporarily empty" window real ones do — and a fix verified against a shallow test dataset can still be wrong at greater depth. Any change to `setFilter` call timing/count in this file needs live verification against a real repro project at real depth, not just mocked unit tests, before being trusted. + +## Decisions log + +- 2026-09-15 (WC-3564): initially decided to wire up `props.hasChildren` as the icon-visibility source. **Reverted after live testing crashed the widget** — discovered `hasChildren` is architecturally unavailable in v2 (see above). Corrected to derive `hasChildren` from `node.children.length > 0` (the pre-existing, structurally-correct signal) and drive the spinner from `datasource.status` instead of any per-node stored state. +- 2026-09-15 (WC-3564, found during manual verification): discovered and fixed two further pre-existing bugs in `useInfiniteTreeNode.ts`'s one-level-lookahead preload (see above) — bundled into the same change with explicit user sign-off, since found/understood/fixed during the same verification pass. The second one required three attempts: a fire-count-based cap broke live testing and was reverted; a content-based 2-round cap fixed that but was itself too shallow against deeper data; an unbounded content-gated cascade, scoped to `startExpanded = Yes` only (confirmed via explicit question — user rejected applying it to `startExpanded = No` too, since that would eagerly prefetch descendants of still-collapsed, not-yet-visible branches), fixed it correctly at real depth, verified live. diff --git a/packages/pluggableWidgets/tree-node-web/openspec/changes/fix-tree-node-loading-state/.openspec.yaml b/packages/pluggableWidgets/tree-node-web/openspec/changes/fix-tree-node-loading-state/.openspec.yaml new file mode 100644 index 0000000000..96db9a43b6 --- /dev/null +++ b/packages/pluggableWidgets/tree-node-web/openspec/changes/fix-tree-node-loading-state/.openspec.yaml @@ -0,0 +1,2 @@ +schema: spec-driven +created: 2026-09-15 diff --git a/packages/pluggableWidgets/tree-node-web/openspec/changes/fix-tree-node-loading-state/design.md b/packages/pluggableWidgets/tree-node-web/openspec/changes/fix-tree-node-loading-state/design.md new file mode 100644 index 0000000000..5cd3aff415 --- /dev/null +++ b/packages/pluggableWidgets/tree-node-web/openspec/changes/fix-tree-node-loading-state/design.md @@ -0,0 +1,79 @@ +## Context + +`TreeNode.tsx` (root dispatcher) routes to v2 (`src/components/v2/`) whenever `parentAssociation` is configured — the self-referencing "infinite tree" mode this ticket's repro projects use. `TreeNode.editorConfig.ts:38-39` hides the `hasChildren` widget property from Studio Pro whenever `parentAssociation` is set, and it has no XML default value — so on every v2 instance, `props.hasChildren` is `undefined` at runtime. (Confirmed live: an initial attempt to read it crashed the widget with `TypeError: Cannot read properties of undefined (reading 'get')`.) v2 must derive "does this node have children" structurally, from `node.children.length > 0` — there is no other signal available to it. + +Why this requires fetching at all, rather than a cheap existence check: Mendix's pluggable-widget data API (`ListValue`) gives a widget exactly one shared, filter-driven list — there is no lighter-weight "does association X have any related record" or per-item count primitive exposed to pluggable widgets. Determining "does node N have children" means asking the datasource for items whose parent is N and checking whether anything comes back; there's no way to get that answer without the datasource actually returning (at least) the matching item(s). This is why the whole preload mechanism (`loadedParentsByIdRef`/`loadedChildsByIdRef`, both in `appendItems` and the bootstrap effect below) exists at all — it's not a workaround for a missed optimization, it's the only mechanism this API surface provides for v2's configuration mode. + +`useIncrementalTreeData.ts:114-119` (pre-fix) flipped a node out of `TreeNodeState.LOADING` the moment its id reappeared in _any_ subsequent `items` delivery — not specifically a delivery meant to answer "does this node have children." A microflow datasource (`useInfiniteTreeNode.ts`) always redelivers its full flattened result regardless of the filter passed to `datasource.setFilter(...)`, since microflow datasources ignore filters entirely. Two consequences, confirmed via live instrumented repro against customer-attached repro projects for WC-3564: + +- **Bug 1**: with "Start expanded" = Yes, a node stays `LOADING` forever if the only mechanism meant to resolve it (a filtered re-delivery) never distinguishes "answered" from "not yet answered" — the spinner never clears. +- **Bug 2**: with "Start expanded" = No, expanding any node triggers `appendItems` → `setFilter`, which (because the microflow ignores the filter) redelivers everything, including the ids of _unrelated_, not-yet-clicked nodes. Those nodes flip out of `LOADING` per lines 114-119, landing in `COLLAPSED_WITH_JS` with an empty `children` array — permanently killing their icon, since the icon-render condition (`TreeNode.tsx:49`, pre-fix) was `hasChildren || treeNodeState === LOADING` and neither is true anymore. + +Git history (`dcbf9bdb51`, "fix: add empty message, loading, and keyboard nav") shows `LOADING` was added later, purely for polish: before that commit, a new node was created directly as `EXPANDED`/`COLLAPSED_WITH_JS`, and a node that would turn out to have children simply showed no icon for one render until its children got placed — a minor "icon pops in" flicker. `LOADING` was introduced to bridge that instant with a spinner instead of nothing, but the _resolution_ criterion it shipped with was wrong, and that wrong criterion is the root cause of both WC-3564 bugs. + +## Goals / Non-Goals + +**Goals:** + +- Expand-icon visibility for v2 must never depend on a widget property that is architecturally unavailable in v2's own configuration mode (`hasChildren`). +- A node's spinner state must never be "stuck" (Bug 1) or capable of corrupting an unrelated node's state (Bug 2). +- Preserve the original polish goal (avoid an abrupt icon pop-in) using a signal that cannot exhibit either bug. +- Both WC-3564 bugs fixed by the same underlying mechanism (not two separate patches). + +**Non-Goals:** + +- Not changing v1 behavior — v1 correctly reads `hasChildren` (it has no association-based alternative) and is untouched by this change. +- Not fixing incorrect Studio Pro configuration of `hasChildren` — irrelevant to v2 now, since v2 never reads it. +- Not guaranteeing a spinner ever shows for every conceivable timing gap — the fix only guarantees the spinner is never stuck and never corrupts a sibling; if the datasource's `status` never reports `Loading` for a given fetch, no spinner shows for it (functionally harmless, matches original pre-`LOADING`-commit behavior). + +## Decisions + +### D1 (revised): `hasChildren` stays derived from `node.children.length > 0`; `props.hasChildren` is never read in v2 + +Initial design used `props.hasChildren.get(node.item).value` as the icon-visibility source, on the premise that the prop was simply unused, not unusable. Live testing against the actual WC-3564 repro project crashed the widget (`props.hasChildren` is `undefined` for any v2 instance — see Context). Reverted: `TreeNode.tsx`'s `hasChildren` local goes back to `node.children.length > 0`, exactly as before this ticket. `renderRecursiveNode` no longer threads a `hasChildren` expression parameter at all. + +This is safe against both bugs once D2 (below) lands, because `node.children` is only ever mutated by real, structurally-correct placement (`useIncrementalTreeData.ts`'s `placeNode`) — there is no longer a spurious "resolve" step that can zero out a node's children array based on unrelated data. + +### D2 (revised): `LOADING` is a render-time-only spinner decision, never stored per node + +A newly-created node is created directly as `EXPANDED`/`COLLAPSED_WITH_JS` (per `config.startExpanded`) in `useIncrementalTreeData.ts` — `TreeNodeState.LOADING` is never assigned to `treeNodeState` anywhere in that file, and the click handler in `TreeNode.tsx` goes back to unconditionally setting `EXPANDED` (matching pre-`LOADING`-commit behavior; no guard needed since nothing sets `LOADING` on click anymore). + +The spinner is computed fresh on every render in `TreeNode.tsx`: `showSpinner = node.children.length === 0 && props.datasource.status === ValueStatus.Loading`. `datasource.status` is Mendix's own first-party, real-time "is this datasource actually fetching right now" signal (`ListValue.status: ValueStatus`) — not bookkeeping we maintain ourselves. `renderHeaderIcon` receives `showSpinner ? TreeNodeState.LOADING : node.treeNodeState`, so `LOADING` still exists as an icon-rendering signal (satisfying the original "repurpose, don't remove" call), just never persisted on the node. + +This is structurally immune to both bugs: + +- **Bug 1 can't recur**: nothing is ever "waiting" in a stored sense. The spinner shows exactly while `status` says `Loading`, and clears the instant it doesn't — including the case where a microflow's `setFilter` call is a genuine no-op and `status` never even transitions to `Loading` (spinner correctly never shows, rather than showing forever). +- **Bug 2 can't recur**: there's no per-node mutable "resolved" flag to wrongly flip. Every render recomputes `hasChildren` fresh from the current `children` array and `showSpinner` fresh from the single global `status` flag — one node's expand action can only ever change _its own_ children array (via real placement) or the shared `status` (which, if it flips, affects all unresolved nodes' spinners equally and correctly, not selectively/incorrectly). + +**Alternatives considered**: + +- _Track per-node fetch-request ids from `useInfiniteTreeNode.ts` and gate resolution on that._ Rejected — solves the wrong layer; still lets a node start `LOADING` and wait indefinitely if the tracked request never resolves (Bug 1's actual mechanism), and adds bookkeeping complexity for no additional correctness over the `datasource.status` approach. +- _Resolve `LOADING` immediately whenever a node's first child is placed, entered via click when `children.length === 0`._ Rejected — requires guarding the click handler against genuine leaves (ambiguous: `children.length === 0` means both "confirmed leaf" and "real parent not yet preloaded," indistinguishable from node state alone), and turned out to be moot anyway once `hasChildren` was reverted to being structurally-derived (a node is only ever clickable once its children are already known, via the existing one-level-lookahead preload — see Context). +- _Drop `LOADING`/the spinner entirely._ Considered when it looked like the preload design left no genuine "waiting" window at all. Rejected per explicit user request to keep a spinner "just in case" — `datasource.status` gives a correct way to do that without reintroducing either bug. + +### D3 (added — found during manual verification, not part of the original two bugs): the one-level-lookahead preload had two gaps, both closed + +While verifying D1/D2 live, manual testing surfaced that a node's expand affordance for a _deeper_ tier sometimes didn't appear until the user collapsed and re-expanded a node — a real, pre-existing bug on `main`, unrelated to the `LOADING`/`hasChildren` mechanism above (it lives entirely in `useInfiniteTreeNode.ts`, which D1/D2 never touch). Two separate gaps in the same "preload one level past what's currently expanded" mechanism: + +- **Click-driven gap** (`appendItems`): the grandchildren-preload step (`children.forEach(...)`, adding a node's children to `loadedChildsByIdRef` so _their_ children get fetched too) was nested inside `if (loadedParentsByIdRef.current.has(parentId))` — true only from a node's _second_ expand onward. A node's first-ever expand skipped preloading its children's children, so a deeper tier's expand icon only appeared after a collapse+re-expand. **Fix**: removed that outer gate — the preload step now always runs when children are passed in, regardless of whether this is the first or a later expand. Verified live: a single click now reveals a 4th tier that previously needed collapse+re-expand. +- **Bootstrap-path gap** (the second `useEffect`, `startExpanded = Yes` specifically): root nodes auto-expand via a separate path that populates `loadedParentsByIdRef` directly from `datasource.items`, bypassing `appendItems` entirely — so the click-driven fix above doesn't reach them. This path was also capped at exactly one automatic round (`if (loadedParentsByIdRef.current.size === 0)`, true only once ever), so it preloaded roots' children but never went one level further. + + **First attempt, reverted after breaking live**: replaced the one-shot gate with a `bootstrapRoundRef` counter that advanced unconditionally on every effect firing, capped at 2. Passed unit tests against a mocked datasource, but broke the real "Expanded bug" tab live — the tree stopped rendering anything past the root level. Root-caused with temporary per-widget-tagged debug instrumentation (three tree widgets mount simultaneously on that page regardless of which tab is active, so untagged logs were unreadable): both rounds fired, and _locked themselves in_, while `datasource.items` was still transiently empty during initial load — before the real root items ever arrived. A blind counter can't distinguish "this effect fired" from "this effect fired with something worth preloading"; it burned both capped rounds on nothing, permanently disabling the mechanism. + + **Second attempt**: replaced the counter with two content-based flags (`round1DoneRef`, `round2DoneRef`) that only flip once real, not-yet-tracked items are actually found — mirroring the pre-existing round-1 gate's own self-correcting semantics (`loadedParentsByIdRef.current.size === 0`, checked _after_ attempting to populate: harmlessly retries on an empty delivery, locks in only once real data lands). Round 2 only locks in once its scan finds at least one item that isn't already a known parent or child. Verified live: the "Expanded bug" tab (2-tier test data at the time) showed all tiers automatically on load, with no regressions to Bug 1/Bug 2/the `appendItems` fix. + + **Third attempt (final, kept)**: against a deeper (4-tier) dataset, the 2-round cap turned out insufficient — the 3rd tier appeared as visible content but without its own expand icon, needing one more real click on an ancestor to reveal, one level deeper than the original repro exposed. Root insight: under `startExpanded = Yes`, _every_ level defaults to `EXPANDED`, not just roots (`useIncrementalTreeData.ts`'s node-creation branch — see D2) — so every level needs the same automatic one-level-lookahead treatment, not a fixed count of 2. The original "avoid eagerly walking the whole tree" concern (below) doesn't actually apply to `startExpanded = Yes`: since nothing is collapsed in that mode, walking the whole tree _is_ the correct, intended behavior — bounded by the tree's real depth (a finite, self-terminating cascade), not an unbounded/runaway one. Fixed by replacing the 2-round cap with an unbounded cascade, gated specifically on `startExpanded === true`: keep treating newly-arrived items as loaded-parents and fetching their children for as long as new descendants keep appearing; stop once a round finds nothing new. `startExpanded = false` keeps the original capped round1+round2 behavior — for that mode, deeper tiers are still collapsed by default and resolve correctly via a single real click already (group 6's fix), so auto-cascading further would only be wasted eager-fetching of not-yet-visible content. Verified live: the "Expanded bug" tab (now the real 4-tier dataset) shows every tier automatically, matching the exact result the user originally showed as expected; the "Collapsed bug" tab's 2-round-capped behavior is unchanged and still passes. + +**Alternative considered**: apply the unbounded cascade regardless of `startExpanded`. Rejected per explicit user decision — would eagerly prefetch descendants of branches that are still collapsed and not visible under `startExpanded = false`, for no user-visible benefit (those tiers already resolve correctly in a single click once actually expanded). +**Alternative considered** (superseded by the "why can't we just know without fetching" question — see Context below): skip preloading and derive "has children" from a cheaper existence check. There is no such check available — see Context. + +## Risks / Trade-offs + +- **[Trade-off]** If a real (non-microflow) datasource's `status` never meaningfully transitions to `Loading` for some fetch (e.g. resolves synchronously from cache), the spinner simply won't show for that fetch — same as the original pre-`LOADING`-commit behavior (brief icon pop-in instead of a spinner). Cosmetic only, not a functional regression. +- **[Risk]** None identified that could reproduce either original bug — see D2's "structurally immune" reasoning above. Covered by unit tests asserting spinner-shows-while-loading, spinner-clears-on-settle (whether or not children arrived), and one node's resolution never affecting a sibling's spinner/children/state. +- **[Risk]** D3's bootstrap round-2 preload fetches one extra level for every currently-known item, regardless of whether the user has looked at it — a bounded, one-time eager-fetch (proportional to tree _width_ at that level, not depth) → **Mitigation**: capped at exactly 2 rounds via content-based flags, verified live and by a unit test asserting a 3rd datasource change does not trigger a 3rd round. This matches the cost the widget already pays for round 1 (unconditionally preloading roots' children regardless of collapse state) — round 2 is the same category of cost, one level deeper, not a new category of risk. +- **[Risk — realized and fixed, kept as a lesson]** A first attempt at this exact fix (a fire-count-based cap) passed every unit test against a mocked datasource but broke a real repro project live, because mocked datasources don't reproduce the transient "still loading, items temporarily empty" window that real ones do. Mitigation going forward: any change to `useInfiniteTreeNode.ts` that touches `setFilter` call timing/count must be live-verified against a real datasource before being trusted, not just unit-tested against a mock. + +## Migration Plan + +No data or config migration. `hasChildren` is no longer read by v2 at all, so existing v2 configurations (where it was always hidden/unset anyway) are unaffected; v1 is untouched. Standard widget version bump + changelog entry per repo convention; no feature flag needed since this is a bug fix restoring intended behavior. diff --git a/packages/pluggableWidgets/tree-node-web/openspec/changes/fix-tree-node-loading-state/proposal.md b/packages/pluggableWidgets/tree-node-web/openspec/changes/fix-tree-node-loading-state/proposal.md new file mode 100644 index 0000000000..8a8757f2de --- /dev/null +++ b/packages/pluggableWidgets/tree-node-web/openspec/changes/fix-tree-node-loading-state/proposal.md @@ -0,0 +1,33 @@ +## Why + +Tree Node v2 stores `LOADING` as a per-node state and resolves it via a broken heuristic: "this node's id reappeared in some later datasource delivery." A microflow datasource — which always redelivers its full flattened result and ignores `setFilter` — breaks that heuristic in two ways (WC-3564): a permanently stuck loading spinner when "Start expanded" is Yes, and a 3rd-tier node silently and permanently losing its expand icon when a sibling is expanded. Both share the same root cause and are fixed by the same change, hence one proposal covering both. + +Manual verification of that fix surfaced two further, separate, pre-existing bugs in the same "preload one level ahead of what's expanded" mechanism (`useInfiniteTreeNode.ts`) — unrelated to `LOADING`/`hasChildren`, but bundled into this same change since they were found, understood, and fixed during the same verification pass: a node's first-ever expand didn't preload its own children's children (needed a collapse+re-expand of that same node to reveal a deeper tier), and the automatic root-expansion path for "Start expanded" = Yes had the same gap, recurring at every level. A first attempt at the second fix (a fixed 2-round cap) broke a live repro project and was reverted before being corrected; a second attempt fixed that but, against a deeper (4-tier) dataset, turned out to still be too shallow — every level defaults to expanded under "Start expanded" = Yes, not just roots, so a fixed round count can't be right at all. The final fix replaces the round cap with an unbounded, self-terminating cascade, scoped specifically to "Start expanded" = Yes. See `design.md` D3 for the full story, including the lesson that a fix here needs live verification against a real datasource, not just mocked unit tests. + +## What Changes + +- `LOADING` is no longer stored on a node at all. Nodes are created already resolved (`EXPANDED`/`COLLAPSED_WITH_JS` per `startExpanded`); the click-to-expand handler goes back to unconditionally setting `EXPANDED`. +- The spinner becomes a pure render-time decision: shown when a node has no known children yet (`node.children.length === 0`) **and** Mendix's own `datasource.status === ValueStatus.Loading` — a real, first-party "is this actually fetching right now" signal, not per-node bookkeeping. +- Expand-icon visibility for v2 stays derived from `node.children.length > 0` (unchanged from before this ticket) — **not** from the `hasChildren` widget property. `hasChildren` is hidden by Studio Pro and unset at runtime whenever `parentAssociation` is configured, which is every v2 instance; reading it crashes the widget (confirmed live during this change's implementation). +- `useInfiniteTreeNode.ts`'s `appendItems`: removed a gate that skipped preloading a node's grandchildren-existence on its first-ever expand (only ran from the second expand onward). +- `useInfiniteTreeNode.ts`'s bootstrap effect: when "Start expanded" is Yes, the preload now cascades level-by-level for as long as new descendants keep appearing (self-terminating once a level introduces nothing new) — matching every level defaulting to expanded in that mode. When "Start expanded" is No, the original capped round1+round2 behavior is unchanged (deeper tiers stay collapsed by default and already resolve correctly via a single real click). +- Update `TreeNodeV2.spec.tsx`, `useIncrementalTreeData.spec.ts`, and `useInfiniteTreeNode.spec.ts` to cover the corrected behavior. + +## Capabilities + +### New Capabilities + +- `tree-node-expand-state`: governs how a v2 tree node decides (a) whether it shows an expand affordance at all, and (b) whether it shows a spinner in place of that affordance, independent of datasource re-delivery timing or datasource type (microflow vs. non-microflow). + +### Modified Capabilities + +(none — no existing `openspec/specs/` in this package prior to this change) + +## Impact + +- `src/components/v2/TreeNode.tsx` — icon-render condition now spinner-vs-chevron based on `datasource.status`; click handler simplified (no `LOADING` entry). +- `src/components/v2/hooks/useIncrementalTreeData.ts` — nodes created pre-resolved; no `LOADING` assignment anywhere in this file. +- `src/components/v2/hooks/useInfiniteTreeNode.ts` — `appendItems`'s preload gate removed; bootstrap effect's preload is now an unbounded, content-gated cascade when `startExpanded` is Yes, and unchanged (capped round1+round2) when it's No. +- `typings/TreeNodeProps.d.ts` — no change. +- `src/components/v2/__tests__/TreeNodeV2.spec.tsx`, `src/components/v2/hooks/__tests__/useIncrementalTreeData.spec.ts`, `src/components/v2/hooks/__tests__/useInfiniteTreeNode.spec.ts` — updated/added regression tests. +- No XML property changes. `hasChildren` remains untouched for v1 (unaffected by this change). diff --git a/packages/pluggableWidgets/tree-node-web/openspec/changes/fix-tree-node-loading-state/specs/tree-node-expand-state/spec.md b/packages/pluggableWidgets/tree-node-web/openspec/changes/fix-tree-node-loading-state/specs/tree-node-expand-state/spec.md new file mode 100644 index 0000000000..8fb0ba89a2 --- /dev/null +++ b/packages/pluggableWidgets/tree-node-web/openspec/changes/fix-tree-node-loading-state/specs/tree-node-expand-state/spec.md @@ -0,0 +1,85 @@ +## ADDED Requirements + +### Requirement: Expand affordance visibility is driven by known children, not by a load-timing-sensitive stored state + +The v2 Tree Node widget SHALL determine whether a node's expand affordance (chevron/icon) is shown based on whether that node currently has any children placed under it (`node.children.length > 0`), computed fresh on every render — never from the `hasChildren` widget property, which is unavailable in v2's configuration mode (`parentAssociation` set), and never from a stored per-node flag that a later, unrelated datasource delivery could incorrectly mutate. + +#### Scenario: A node with children shows an expand affordance + +- **WHEN** a node has at least one child currently placed under it +- **THEN** the widget renders an expand affordance for that node + +#### Scenario: A node with no known children shows no expand affordance (absent a spinner) + +- **WHEN** a node has no children currently placed under it and the datasource is not currently loading +- **THEN** the widget renders no expand affordance for that node + +#### Scenario: Expanding one node does not affect a sibling's expand affordance + +- **WHEN** a user expands node A, causing a datasource redelivery that includes node B's id (node B was never expanded and has no relation to node A) +- **THEN** node B's expand affordance and underlying children are unchanged from before node A was expanded + +### Requirement: A loading spinner is shown only while the datasource is genuinely fetching, never as stored per-node state + +The v2 Tree Node widget SHALL show a loading spinner in place of the expand affordance for a node that has no known children yet, exactly while `datasource.status === ValueStatus.Loading`. This is a render-time computation only — no per-node "is loading" flag is stored, so the spinner cannot become stuck and cannot be affected by an unrelated node's resolution. + +#### Scenario: Spinner shown while the datasource is loading and children are unknown + +- **WHEN** a node has no children placed under it yet and the datasource's `status` is `Loading` +- **THEN** the widget shows a spinner in place of the expand affordance for that node + +#### Scenario: Spinner clears once the datasource settles, regardless of outcome + +- **WHEN** the datasource's `status` transitions away from `Loading` +- **THEN** every node's spinner clears immediately — showing an expand affordance if children arrived, or no affordance at all if they didn't + +#### Scenario: Spinner never shows for a node that already has children + +- **WHEN** a node already has at least one child placed under it +- **THEN** the widget never shows a spinner for that node, regardless of `datasource.status` + +#### Scenario: A stalled or filter-ignoring datasource never produces a stuck spinner + +- **WHEN** a microflow datasource ignores `setFilter` and its `status` never transitions to `Loading` for a given fetch attempt +- **THEN** no node is left showing a spinner indefinitely as a result of that fetch attempt + +### Requirement: A manually expanded node's own children's expand affordance is known without requiring a collapse-and-reopen + +The v2 Tree Node widget SHALL preload one level past a node's children when that node is expanded by a user click, so each child's own expand affordance is already correct the first time its parent is expanded — never requiring the user to collapse and re-expand that same node to reveal it. This preload is bounded to exactly one level past what's already known for this path; it does not eagerly walk the full tree beyond the node that was actually clicked. + +#### Scenario: A node's own children's children are known on its first expand + +- **WHEN** a user expands a node for the first time (its children are already known, but whether those children themselves have children is not) +- **THEN** each of that node's children already shows its correct expand affordance immediately, without requiring that child to be separately collapsed and re-expanded + +### Requirement: Under "Start expanded" = Yes, every auto-expanded level's own expand affordance is known automatically, all the way to the tree's real depth + +Because every node defaults to expanded (not just roots) when "Start expanded" is Yes, the v2 Tree Node widget SHALL keep preloading one level further for as long as new descendants keep appearing — not a fixed number of levels — so that every already-visible node's expand affordance is correct without any manual collapse-and-reopen, regardless of how deep the actual tree data goes. This cascade is self-terminating: it stops automatically once a level introduces no previously-unseen items, bounded by the tree's real depth rather than an arbitrary count or recursing indefinitely. + +#### Scenario: A 3rd (or deeper) tier's own expand affordance is known automatically + +- **WHEN** "Start expanded" is Yes and the underlying data has 3 or more tiers +- **THEN** every tier's nodes show their correct expand affordance immediately on load, with no tier requiring a manual collapse-and-reopen to reveal the next tier down + +#### Scenario: The cascade stops once the real data is exhausted + +- **WHEN** a subsequent datasource delivery introduces no items beyond what's already known +- **THEN** no further automatic preload round is triggered — the cascade does not continue indefinitely or re-fetch unchanged data + +#### Scenario: A transient empty datasource delivery during initial load does not disable the cascade + +- **WHEN** the datasource is still loading and delivers an empty item set one or more times before the real data arrives +- **THEN** the cascade does not lock itself out on that empty delivery — it only advances once it actually finds real, previously-unseen items, and keeps retrying harmlessly until it does + +### Requirement: The auto-cascade does not apply when "Start expanded" is No + +The v2 Tree Node widget SHALL NOT auto-cascade the preload beyond the existing capped behavior (root's children, plus one level of lookahead) when "Start expanded" is No, since deeper tiers remain collapsed by default and already resolve correctly via a single real click. Auto-cascading further in this mode would only eagerly fetch descendants of branches the user has not opened. + +#### Scenario: A 3rd-tier arrival does not trigger a further automatic round when collapsed by default + +- **WHEN** "Start expanded" is No and a 3rd-tier item arrives as a result of the existing 2-round preload +- **THEN** no further automatic preload round is triggered for it — expanding it further still requires a real click + +### Requirement: Auto-expanded root nodes (`startExpanded = Yes`) — NOT YET IMPLEMENTED + +The equivalent one-level lookahead for automatically auto-expanded root nodes (so a root's children already show their correct expand affordance without needing the root collapsed and re-expanded) was attempted and reverted after it broke a live repro project (see `design.md` D3). No requirement is claimed here for this case. A collapse+re-expand of a root node is currently still needed to reveal a 3rd tier under `startExpanded = Yes`; this is a known, pre-existing, unfixed gap, tracked for a future change once root-caused. diff --git a/packages/pluggableWidgets/tree-node-web/openspec/changes/fix-tree-node-loading-state/tasks.md b/packages/pluggableWidgets/tree-node-web/openspec/changes/fix-tree-node-loading-state/tasks.md new file mode 100644 index 0000000000..c0e8656837 --- /dev/null +++ b/packages/pluggableWidgets/tree-node-web/openspec/changes/fix-tree-node-loading-state/tasks.md @@ -0,0 +1,50 @@ +## 1. Resolve-at-creation, drop stored `LOADING` (D2) + +- [x] 1.1 In `useIncrementalTreeData.ts`'s node-creation branch, create new nodes directly as `config.startExpanded ? TreeNodeState.EXPANDED : TreeNodeState.COLLAPSED_WITH_JS` instead of `TreeNodeState.LOADING`. +- [x] 1.2 Remove the "existing node in `LOADING` resolves on reappearance" branch entirely — no node is ever assigned `LOADING` in this file anymore, so there is nothing left to resolve. +- [x] 1.3 Revert `TreeNode.tsx`'s click handler to unconditionally set `EXPANDED` on expand-click (matches pre-`LOADING`-commit behavior) — no `LOADING` entry via click, no guard needed. +- [x] 1.4 Compute the spinner at render time in `TreeNode.tsx`: `showSpinner = node.children.length === 0 && props.datasource.status === ValueStatus.Loading`. Pass `showSpinner ? TreeNodeState.LOADING : node.treeNodeState` into `renderHeaderIcon`. No per-node `LOADING` is stored anywhere. + +## 2. `hasChildren` derivation — reverted after live-testing crash + +- [x] 2.1 **Correction, found via live Studio Pro testing (not caught by unit tests):** the initial plan was to read `props.hasChildren` in v2. This crashed the widget — `TreeNode.editorConfig.ts:38-39` hides `hasChildren` from Studio Pro whenever `parentAssociation` is configured (every v2 instance, including both of this ticket's repro projects), and it has no XML default, so `props.hasChildren` is `undefined` at runtime for v2. Reverted `TreeNode.tsx:20`'s `hasChildren` derivation back to `node.children.length > 0` — the same signal it used before this ticket. Removed the `hasChildrenExpr` parameter from `renderRecursiveNode` entirely. +- [x] 2.2 Confirmed `aria-expanded`, icon clickability, and the icon-render condition all still read the single `hasChildren` local (now `node.children.length > 0`) unchanged. +- [x] 2.3 Confirmed the icon-render condition is `(hasChildren || showSpinner) && iconPlacement !== "no"` — a node with children never spins; a childless node spins only while `datasource.status === Loading`. + +## 3. Regression tests + +- [x] 3.1 Rewrote `TreeNodeV2.spec.tsx`'s stale comment (previously claimed the datasource setup existed to satisfy a `hasChildren` prop check — no longer applicable since `hasChildren` isn't read at all). Hoisted shared test helpers to module scope so a new describe block could reuse them. +- [x] 3.2 Added a unit test (`TreeNodeV2.spec.tsx` + `useIncrementalTreeData.spec.ts`): a node is created directly in `EXPANDED`/`COLLAPSED_WITH_JS` per `startExpanded` and never passes through `LOADING`, even when the datasource keeps redelivering the same full item set (simulating a microflow ignoring `setFilter`) — Bug 1 regression. +- [x] 3.3 Added a unit test: a node's own children arriving does not change an unrelated sibling's `treeNodeState`, children, or spinner — Bug 2 regression. Covered in both spec files. +- [x] 3.4 Added unit tests for the spinner itself: shows while `datasource.status === Loading` and no children are known; clears once `status` settles regardless of whether children arrived; never shows for a node that already has children. + +## 4. Manual verification + +- [x] 4.1 Rebuilt against `~/Documents/_tickets/WC-3564-2` and drove it with Playwright. First rebuild (with the `props.hasChildren`-based design) crashed the widget live — see task 2.1. After the correction, rebuilt again and confirmed: expanding "Top level 2" leaves "Second level 1a"/"Second level 1b"'s expand icons intact (icon count unchanged before/after, screenshot-confirmed) — Bug 2 fixed. +- [x] 4.2 Confirmed bug 1's repro (microflow datasource, "Start expanded" = Yes) live: tree renders fully expanded immediately, zero `.widget-tree-node-loading-spinner` elements, no console/page errors. + +## 5. Changelog + +- [x] 5.1 Added a `CHANGELOG.md` entry under `[Unreleased]` describing the user-visible fix (both bugs), no implementation details, per repo changelog conventions. + +## 6. Third finding: `appendItems` off-by-one-click preload gap (found during manual verification) + +Pre-existing on `main`, unrelated to `useIncrementalTreeData.ts`/`TreeNode.tsx` (untouched by groups 1-2). A node's own click-to-expand never preloaded its _own_ children's children — required a collapse+re-expand of that same node before a deeper tier's expand icon appeared. + +- [x] 6.1 In `useInfiniteTreeNode.ts`'s `appendItems`, removed the outer `if (loadedParentsByIdRef.current.has(parentId))` gate around the grandchildren-preload `children.forEach(...)` step — it was skipping that step on a node's _first_ expand (the only time it matters), only running it from the second expand onward. +- [x] 6.2 Added a unit test in `useInfiniteTreeNode.spec.ts` — not needed as a new test; existing "first expansion" describe block continues to cover this since the gate removal doesn't change its assertions, but confirmed no existing test asserted the buggy gated behavior. +- [x] 6.3 Verified live: rebuilt against `~/Documents/_tickets/WC-3564-2`, single click on "Second level 1a" (previously required collapse+re-expand) now immediately reveals "Fourth level 1" under "Third level 1a1" — confirmed via Playwright polling (no click-twice needed). +- [x] 6.4 Re-ran the full rigorous Bug 1 / Bug 2 regression suite live after this change — no regressions. + +## 7. Fourth finding: bootstrap preload capped at one round, never reaches a second (found during manual verification) + +Pre-existing on `main`, in `useInfiniteTreeNode.ts`'s second `useEffect` block — separate from group 6 (that gate was in `appendItems`, click-driven; this one is in the automatic bootstrap path that runs regardless of clicks). For `startExpanded = Yes` specifically, root nodes auto-expand via this bootstrap path rather than via `appendItems`, so group 6's fix doesn't reach them — closing and reopening a root node was required to reveal a 3rd tier. + +- [x] 7.1 **First attempt (reverted):** replaced the one-shot `if (loadedParentsByIdRef.current.size === 0)` gate with a `bootstrapRoundRef` counter that advanced unconditionally on every effect firing, capped at 2. Passed unit tests against a mocked datasource. **Broke live**: rebuilt against `~/Documents/_tickets/WC-3564-2`, the "Expanded bug" tab permanently stopped rendering anything past the root level. Root-caused via temporary debug instrumentation (tagged per-widget-instance to disentangle the 3 tree widgets that mount simultaneously on that page): both rounds fired — and locked themselves in — while `datasource.items` was still transiently empty (still loading), _before_ the real root items ever arrived. The counter had no way to tell "fired" apart from "fired with real data," so it burned both of its capped rounds on nothing. +- [x] 7.2 **Second attempt (this one, kept):** replaced the counter with two content-based booleans (`round1DoneRef`, `round2DoneRef`) that only flip once real, previously-unseen items are actually found — mirroring the original round-1 gate's own self-correcting semantics (`loadedParentsByIdRef.current.size === 0`, checked _after_ attempting to populate, so it harmlessly retries on empty deliveries instead of locking in early). Round 2 only locks in once it finds at least one item that isn't already a known parent or child. +- [x] 7.3 Removed all debug instrumentation added for root-causing 7.1 (tagged `[DEBUG-t3564b]`, per-widget-instance) — confirmed zero references remain. +- [x] 7.4 Added a unit test in `useInfiniteTreeNode.spec.ts` that explicitly exercises the failure mode from 7.1: several transient empty-item rerenders before round 1 locks in, another empty/unchanged rerender before round 2 locks in, then confirms round 2 only advances once real new items appear, and a further identical rerender does not trigger a round 3. +- [x] 7.5 Verified live: rebuilt against `~/Documents/_tickets/WC-3564-2`. "Expanded bug" tab now shows all 3 tiers automatically on load — no manual toggle needed. Re-ran the full rigorous Bug 1 / Bug 2 / group-6 (`appendItems`) regression suite live — all still pass, no regressions. +- [x] 7.6 **Follow-up finding, same session:** the 2-round cap turned out insufficient — with a 4-tier dataset, the 3rd tier appeared as content but without its own expand icon (needed a real click on its own ancestor to reveal, one level deeper than the original repro). Root cause: under `startExpanded = Yes`, _every_ level defaults to `EXPANDED` (not just roots), so every level needs the same automatic preload treatment, not just a fixed 2 rounds. Fixed by replacing the 2-round cap with an unbounded, self-terminating cascade **gated specifically on `startExpanded === true`**: keep treating newly-arrived items as loaded-parents and fetching their children for as long as new descendants keep appearing, stopping naturally once a round finds nothing new (bounded by the tree's real depth, not an arbitrary count). Explicitly scoped to `startExpanded = true` only, per user decision — `startExpanded = false` keeps the original capped round1+round2 behavior unchanged (deeper tiers there already resolve correctly via a single real click, per group 6's fix; auto-cascading for still-collapsed branches would just be wasted eager-fetching of content the user hasn't opened). +- [x] 7.7 Updated the unit test from 7.4 to match: renamed/rewritten as an unbounded-cascade test asserting 3+ sequential levels each trigger exactly one more `setFilter` call as they arrive, and a repeated/empty delivery triggers none. Added a second test confirming `startExpanded = false` still caps at exactly 2 rounds (3rd-tier arrival triggers no further automatic call). +- [x] 7.8 Verified live again: rebuilt against `~/Documents/_tickets/WC-3564-2`. "Expanded bug" tab (4 levels of data) now shows all 4 tiers automatically on load, with `Third level 1a1` already showing its own expand icon with zero manual interaction — matching the exact screenshot the user originally showed as the expected/desired result. Re-ran the full rigorous Bug 1 / Bug 2 / group-6 / "Collapsed bug" (startExpanded=false, 2-round-cap) regression suite live — all still pass, no regressions. diff --git a/packages/pluggableWidgets/tree-node-web/src/components/v2/TreeNode.tsx b/packages/pluggableWidgets/tree-node-web/src/components/v2/TreeNode.tsx index 24ed6171ba..a915bf8a6d 100644 --- a/packages/pluggableWidgets/tree-node-web/src/components/v2/TreeNode.tsx +++ b/packages/pluggableWidgets/tree-node-web/src/components/v2/TreeNode.tsx @@ -15,9 +15,13 @@ function renderRecursiveNode( iconPlacement: TreeNodeContainerProps["showIcon"], openNodeOn: TreeNodeContainerProps["openNodeOn"], onNodeClick: (node: TreeNodeV2DataItem) => void, + isDatasourceLoading: boolean, children?: TreeNodeContainerProps["children"] ): ReactElement { const hasChildren = node.children.length > 0; + // We don't yet know whether this node has children (nothing placed under it yet); show a + // spinner only while the datasource is actually fetching, never as a stored/stale node state. + const showSpinner = !hasChildren && isDatasourceLoading; const isExpanded = node.treeNodeState === TreeNodeState.EXPANDED; const isIconClickable = openNodeOn === "iconClick"; const isHeaderClickable = openNodeOn === "headerClick"; @@ -46,14 +50,14 @@ function renderRecursiveNode( onClick={onHeaderClick} > {node.title} - {(hasChildren || node.treeNodeState === TreeNodeState.LOADING) && iconPlacement !== "no" && ( + {(hasChildren || showSpinner) && iconPlacement !== "no" && ( - {renderHeaderIcon(node.treeNodeState, iconPlacement)} + {renderHeaderIcon(showSpinner ? TreeNodeState.LOADING : node.treeNodeState, iconPlacement)} )} @@ -75,6 +79,7 @@ function renderRecursiveNode( iconPlacement, openNodeOn, onNodeClick, + isDatasourceLoading, children )} @@ -120,6 +125,7 @@ export function TreeNodeV2(props: TreeNodeContainerProps): ReactElement { ); const treeData = useIncrementalTreeData(items, treeConfig); + const isDatasourceLoading = props.datasource.status === ValueStatus.Loading; const onNodeClick = useCallback( (node: TreeNodeV2DataItem) => { if (node.treeNodeState === TreeNodeState.EXPANDED) { @@ -160,6 +166,7 @@ export function TreeNodeV2(props: TreeNodeContainerProps): ReactElement { iconPlacement, props.openNodeOn, onNodeClick, + isDatasourceLoading, props.children ) )} diff --git a/packages/pluggableWidgets/tree-node-web/src/components/v2/__tests__/TreeNodeV2.spec.tsx b/packages/pluggableWidgets/tree-node-web/src/components/v2/__tests__/TreeNodeV2.spec.tsx index 5e378b30d7..474c09d60b 100644 --- a/packages/pluggableWidgets/tree-node-web/src/components/v2/__tests__/TreeNodeV2.spec.tsx +++ b/packages/pluggableWidgets/tree-node-web/src/components/v2/__tests__/TreeNodeV2.spec.tsx @@ -20,72 +20,72 @@ jest.mock("mendix/filters/builders", () => ({ or: jest.fn((...args: unknown[]) => ({ type: "or", args })) })); -describe("TreeNodeV2 - Keyboard Navigation", () => { - const makeItem = (id: string): ObjectItem => ({ id: id as GUID }); - - const makeListValue = (items: ObjectItem[]): ListValue => - ({ - status: ValueStatus.Available, - items, - limit: 100, - offset: 0, - hasMoreItems: false, - sortOrder: [], - filter: undefined, - setLimit: jest.fn(), - setOffset: jest.fn(), - setSortOrder: jest.fn(), - requestTotalCount: jest.fn(), - setFilter: jest.fn(), - reload: jest.fn(), - totalCount: undefined - }) as unknown as ListValue; - - const makeExpression = (value: string): ListExpressionValue => ({ - get: (): DynamicValue => ({ status: ValueStatus.Available, value }) - }); +const makeItem = (id: string): ObjectItem => ({ id: id as GUID }); + +const makeListValue = (items: ObjectItem[]): ListValue => + ({ + status: ValueStatus.Available, + items, + limit: 100, + offset: 0, + hasMoreItems: false, + sortOrder: [], + filter: undefined, + setLimit: jest.fn(), + setOffset: jest.fn(), + setSortOrder: jest.fn(), + requestTotalCount: jest.fn(), + setFilter: jest.fn(), + reload: jest.fn(), + totalCount: undefined + }) as unknown as ListValue; + +const makeExpression = (value: string): ListExpressionValue => ({ + get: (): DynamicValue => ({ status: ValueStatus.Available, value }) +}); - const makeBoolExpression = (value: boolean): ListExpressionValue => ({ - get: (): DynamicValue => ({ status: ValueStatus.Available, value }) - }); +const makeBoolExpression = (value: boolean): ListExpressionValue => ({ + get: (): DynamicValue => ({ status: ValueStatus.Available, value }) +}); - /** - * Creates a ListReferenceValue mock where childId → parentId, all others → undefined. - */ - const makeParentAssociation = (childId: string, parentId: string): ListReferenceValue => - ({ - id: "parentAssoc", - type: "Reference", - get: (item: ObjectItem): DynamicValue => { - if (String(item.id) === childId) { - return { status: ValueStatus.Available, value: makeItem(parentId) }; - } - return { status: ValueStatus.Available, value: undefined as unknown as ObjectItem }; +/** + * Creates a ListReferenceValue mock where childId → parentId, all others → undefined. + */ +const makeParentAssociation = (childId: string, parentId: string): ListReferenceValue => + ({ + id: "parentAssoc", + type: "Reference", + get: (item: ObjectItem): DynamicValue => { + if (String(item.id) === childId) { + return { status: ValueStatus.Available, value: makeItem(parentId) }; } - }) as unknown as ListReferenceValue; - - /** - * Default props for tests that need a node with children. - * Datasource contains parent + child; parentAssociation links child → parent. - * This makes node.children.length > 0 so aria-expanded is rendered. - */ - const makeDefaultProps = (startExpanded = false): TreeNodeContainerProps => ({ - name: "treeNode", - class: "", - tabIndex: 0, - advancedMode: false, - datasource: makeListValue([makeItem("1"), makeItem("2")]), - parentAssociation: makeParentAssociation("2", "1"), - headerType: "text", - headerCaption: makeExpression("Node"), - hasChildren: makeBoolExpression(true), - showIcon: "right", - openNodeOn: "headerClick", - animate: false, - animateIcon: false, - startExpanded - }); + return { status: ValueStatus.Available, value: undefined as unknown as ObjectItem }; + } + }) as unknown as ListReferenceValue; + +/** + * Default props for tests that need a node with children. + * `hasChildren` (not the datasource) is what drives aria-expanded; the + * datasource's parent + child items exist so the expanded body actually renders content. + */ +const makeDefaultProps = (startExpanded = false): TreeNodeContainerProps => ({ + name: "treeNode", + class: "", + tabIndex: 0, + advancedMode: false, + datasource: makeListValue([makeItem("1"), makeItem("2")]), + parentAssociation: makeParentAssociation("2", "1"), + headerType: "text", + headerCaption: makeExpression("Node"), + hasChildren: makeBoolExpression(true), + showIcon: "right", + openNodeOn: "headerClick", + animate: false, + animateIcon: false, + startExpanded +}); +describe("TreeNodeV2 - Keyboard Navigation", () => { it("expands node when Enter key is pressed", () => { render(createElement(TreeNodeV2, makeDefaultProps(false))); const treeItem = screen.getAllByRole("treeitem")[0]; @@ -204,3 +204,106 @@ describe("TreeNodeV2 - Keyboard Navigation", () => { expect(parentItem.getAttribute("aria-expanded")).toBe(initialState); }); }); + +describe("TreeNodeV2 - Loading state (WC-3564 regressions)", () => { + const spinner = (container: HTMLElement): Element | null => + container.querySelector(".widget-tree-node-loading-spinner"); + + const makeListValueWithStatus = (items: ObjectItem[], status: ValueStatus): ListValue => + ({ ...makeListValue(items), status }) as unknown as ListValue; + + it("never shows a stuck spinner, even when the datasource keeps redelivering the same full item set (Bug 1)", () => { + // "1" genuinely has a child ("2"), so its expand affordance should resolve immediately, not depend on a later delivery. + const props: TreeNodeContainerProps = { + ...makeDefaultProps(true), + datasource: makeListValue([makeItem("1"), makeItem("2")]), + parentAssociation: makeParentAssociation("2", "1") + }; + + const { container, rerender } = render(createElement(TreeNodeV2, props)); + expect(spinner(container)).toBeNull(); + expect(screen.getAllByRole("treeitem")[0]).toHaveAttribute("aria-expanded", "true"); + + // Simulate a microflow datasource ignoring setFilter and redelivering + // the exact same full result on a later render (new array reference). + rerender( + createElement(TreeNodeV2, { + ...props, + datasource: makeListValue([makeItem("1"), makeItem("2")]) + }) + ); + + expect(spinner(container)).toBeNull(); + expect(screen.getAllByRole("treeitem")[0]).toHaveAttribute("aria-expanded", "true"); + }); + + it("shows a spinner while the datasource is actually loading and no children are known yet", () => { + const noParent = makeParentAssociation("__none__", "__none__"); + const props: TreeNodeContainerProps = { + ...makeDefaultProps(false), + datasource: makeListValueWithStatus([makeItem("1")], ValueStatus.Loading), + parentAssociation: noParent + }; + + const { container } = render(createElement(TreeNodeV2, props)); + expect(spinner(container)).not.toBeNull(); + expect(screen.getByRole("treeitem")).not.toHaveAttribute("aria-expanded"); + }); + + it("clears the spinner once the datasource settles, even if it turns out the node has no children", () => { + const noParent = makeParentAssociation("__none__", "__none__"); + const props: TreeNodeContainerProps = { + ...makeDefaultProps(false), + datasource: makeListValueWithStatus([makeItem("1")], ValueStatus.Loading), + parentAssociation: noParent + }; + + const { container, rerender } = render(createElement(TreeNodeV2, props)); + expect(spinner(container)).not.toBeNull(); + + rerender( + createElement(TreeNodeV2, { + ...props, + datasource: makeListValueWithStatus([makeItem("1")], ValueStatus.Available) + }) + ); + + expect(spinner(container)).toBeNull(); + expect(screen.getByRole("treeitem")).not.toHaveAttribute("aria-expanded"); + }); + + it("resolving one node's children does not affect an unrelated sibling's spinner or state (Bug 2)", () => { + const parentAssociation = makeParentAssociation("C", "A"); + const props: TreeNodeContainerProps = { + ...makeDefaultProps(false), + datasource: makeListValueWithStatus([makeItem("A"), makeItem("B")], ValueStatus.Loading), + parentAssociation + }; + + const { container, rerender } = render(createElement(TreeNodeV2, props)); + const [nodeA, nodeB] = screen.getAllByRole("treeitem"); + expect(nodeA).not.toHaveAttribute("aria-expanded"); + expect(nodeB).not.toHaveAttribute("aria-expanded"); + // Both spin while nothing is known yet and the datasource is loading. + expect(container.querySelectorAll(".widget-tree-node-loading-spinner")).toHaveLength(2); + + // The datasource settles, delivering a child for A only. B was never involved. + rerender( + createElement(TreeNodeV2, { + ...props, + datasource: makeListValueWithStatus( + [makeItem("A"), makeItem("B"), makeItem("C")], + ValueStatus.Available + ) + }) + ); + + const [nodeAAfter, nodeBAfter] = screen.getAllByRole("treeitem"); + expect(nodeAAfter).toHaveAttribute("aria-expanded", "false"); + expect(nodeAAfter.querySelector(".widget-tree-node-branch-header-icon-container")).not.toBeNull(); + // B has no children and the datasource is no longer loading — no icon, no spinner, untouched by A's resolution. + expect(nodeBAfter).not.toHaveAttribute("aria-expanded"); + expect(nodeBAfter.querySelector(".widget-tree-node-branch-header-icon-container")).toBeNull(); + expect(container.querySelectorAll(".widget-tree-node-loading-spinner")).toHaveLength(0); + }); +}); diff --git a/packages/pluggableWidgets/tree-node-web/src/components/v2/hooks/__tests__/useIncrementalTreeData.spec.ts b/packages/pluggableWidgets/tree-node-web/src/components/v2/hooks/__tests__/useIncrementalTreeData.spec.ts index 598aeff794..27ef1471fd 100644 --- a/packages/pluggableWidgets/tree-node-web/src/components/v2/hooks/__tests__/useIncrementalTreeData.spec.ts +++ b/packages/pluggableWidgets/tree-node-web/src/components/v2/hooks/__tests__/useIncrementalTreeData.spec.ts @@ -70,7 +70,7 @@ describe("useIncrementalTreeData", () => { expect(result.current[0].children[0].id).toBe("child"); }); - it("assigns LOADING on first render, then COLLAPSED_WITH_JS when startExpanded is false", () => { + it("assigns COLLAPSED_WITH_JS on first render when startExpanded is false, and never enters LOADING on redelivery (WC-3564 Bug 1)", () => { const items = [makeItem("a")]; const config = makeConfig({ startExpanded: false }); const { result, rerender } = renderHook( @@ -78,13 +78,13 @@ describe("useIncrementalTreeData", () => { useIncrementalTreeData(items, config), { initialProps: { items, config } } ); - expect(result.current[0].treeNodeState).toBe(TreeNodeState.LOADING); - // Simulate Mendix re-providing items (new array reference) + expect(result.current[0].treeNodeState).toBe(TreeNodeState.COLLAPSED_WITH_JS); + // Simulate a microflow datasource redelivering the same full result (new array reference). rerender({ items: [...items], config }); expect(result.current[0].treeNodeState).toBe(TreeNodeState.COLLAPSED_WITH_JS); }); - it("assigns LOADING on first render, then EXPANDED when startExpanded is true", () => { + it("assigns EXPANDED on first render when startExpanded is true, and stays EXPANDED on redelivery (WC-3564 Bug 1)", () => { const items = [makeItem("a")]; const config = makeConfig({ startExpanded: true }); const { result, rerender } = renderHook( @@ -92,11 +92,31 @@ describe("useIncrementalTreeData", () => { useIncrementalTreeData(items, config), { initialProps: { items, config } } ); - expect(result.current[0].treeNodeState).toBe(TreeNodeState.LOADING); - // Simulate Mendix re-providing items (new array reference) + expect(result.current[0].treeNodeState).toBe(TreeNodeState.EXPANDED); + // Simulate a microflow datasource redelivering the same full result (new array reference). rerender({ items: [...items], config }); expect(result.current[0].treeNodeState).toBe(TreeNodeState.EXPANDED); }); + + it("leaves an unrelated sibling's state untouched when a node's children arrive later (WC-3564 Bug 2)", () => { + const parent = makeItem("parent"); + const sibling = makeItem("sibling"); + const config = makeConfigWithParentMap({ child: "parent" }, { startExpanded: false }); + + const { result, rerender } = renderHook( + ({ items }: { items: ObjectItem[] }) => useIncrementalTreeData(items, config), + { initialProps: { items: [parent, sibling] } } + ); + + expect(result.current.find(n => n.id === "parent")!.treeNodeState).toBe(TreeNodeState.COLLAPSED_WITH_JS); + expect(result.current.find(n => n.id === "sibling")!.treeNodeState).toBe(TreeNodeState.COLLAPSED_WITH_JS); + + // "parent"'s child arrives later; "sibling" was never involved. + rerender({ items: [parent, sibling, makeItem("child")] }); + expect(result.current.find(n => n.id === "parent")!.children).toHaveLength(1); + expect(result.current.find(n => n.id === "sibling")!.treeNodeState).toBe(TreeNodeState.COLLAPSED_WITH_JS); + expect(result.current.find(n => n.id === "sibling")!.children).toHaveLength(0); + }); }); describe("out-of-order arrival (child before parent)", () => { diff --git a/packages/pluggableWidgets/tree-node-web/src/components/v2/hooks/__tests__/useInfiniteTreeNode.spec.ts b/packages/pluggableWidgets/tree-node-web/src/components/v2/hooks/__tests__/useInfiniteTreeNode.spec.ts index e46cc2e403..df9b2539ba 100644 --- a/packages/pluggableWidgets/tree-node-web/src/components/v2/hooks/__tests__/useInfiniteTreeNode.spec.ts +++ b/packages/pluggableWidgets/tree-node-web/src/components/v2/hooks/__tests__/useInfiniteTreeNode.spec.ts @@ -183,4 +183,82 @@ describe("useInfiniteTreeNodes", () => { expect(props.datasource.setFilter).toHaveBeenCalledTimes(2); }); }); + + describe("unbounded cascade when startExpanded is true (WC-3564)", () => { + it("keeps cascading level by level for as long as new descendants appear, and stops once a level is empty — never locking in on a transient empty delivery", () => { + const rootItems = [makeItem("root1"), makeItem("root2")]; + const withSecondTier = [...rootItems, makeItem("second1")]; + const withThirdTier = [...withSecondTier, makeItem("third1")]; + let items: ObjectItem[] = []; + const props = makeProps({ startExpanded: true }); + const setFilterSpy = props.datasource.setFilter as jest.Mock; + + const { rerender } = renderHook(() => + useInfiniteTreeNodes({ ...props, datasource: { ...props.datasource, items } as any }) + ); + expect(setFilterSpy).toHaveBeenCalledTimes(0); // startExpanded skips the initial root-only filter + + // Datasource stays empty across a few transient renders (still loading) — must not + // call setFilter on empty data (this is exactly what broke live testing with a naive + // fire-count-based cap instead of a content-based one). + rerender(); + rerender(); + expect(setFilterSpy).toHaveBeenCalledTimes(0); + + // Real root items arrive — cascades to fetch their children. + items = rootItems; + rerender(); + expect(setFilterSpy).toHaveBeenCalledTimes(1); + + // An unchanged redelivery of the same roots must not trigger another call. + items = rootItems; + rerender(); + expect(setFilterSpy).toHaveBeenCalledTimes(1); + + // Second tier arrives — cascades one level further automatically (no click involved). + items = withSecondTier; + rerender(); + expect(setFilterSpy).toHaveBeenCalledTimes(2); + + // Third tier arrives — keeps cascading (this is the exact scenario that was broken: + // every level defaults to EXPANDED under startExpanded=true, so every level needs its + // own affordance pre-checked, not just roots + one bonus level). + items = withThirdTier; + rerender(); + expect(setFilterSpy).toHaveBeenCalledTimes(3); + + // Nothing new this time (same set redelivered) — stops here, no further call. + items = [...withThirdTier]; + rerender(); + expect(setFilterSpy).toHaveBeenCalledTimes(3); + }); + + it("stays capped at 2 rounds when startExpanded is false — only roots auto-expand, deeper tiers resolve via a real click", () => { + const rootItems = [makeItem("root1")]; + const withSecondTier = [...rootItems, makeItem("second1")]; + const withThirdTier = [...withSecondTier, makeItem("third1")]; + let items: ObjectItem[] = []; + const props = makeProps({ startExpanded: false }); + const setFilterSpy = props.datasource.setFilter as jest.Mock; + + const { rerender } = renderHook(() => + useInfiniteTreeNodes({ ...props, datasource: { ...props.datasource, items } as any }) + ); + expect(setFilterSpy).toHaveBeenCalledTimes(1); // initial root-only filter + + items = rootItems; + rerender(); // round 1 locks in + expect(setFilterSpy).toHaveBeenCalledTimes(2); + + items = withSecondTier; + rerender(); // round 2 locks in + expect(setFilterSpy).toHaveBeenCalledTimes(3); + + // Third tier arriving must NOT trigger a further automatic round — unlike + // startExpanded=true, deeper tiers here only resolve via a real click (appendItems). + items = withThirdTier; + rerender(); + expect(setFilterSpy).toHaveBeenCalledTimes(3); + }); + }); }); diff --git a/packages/pluggableWidgets/tree-node-web/src/components/v2/hooks/useIncrementalTreeData.ts b/packages/pluggableWidgets/tree-node-web/src/components/v2/hooks/useIncrementalTreeData.ts index bbf5f4721e..27c42b4146 100644 --- a/packages/pluggableWidgets/tree-node-web/src/components/v2/hooks/useIncrementalTreeData.ts +++ b/packages/pluggableWidgets/tree-node-web/src/components/v2/hooks/useIncrementalTreeData.ts @@ -111,12 +111,6 @@ export function useIncrementalTreeData(items: ObjectItem[] | undefined, config: placeNode(existingNode); } - if (existingNode.treeNodeState === TreeNodeState.LOADING) { - existingNode.treeNodeState = config.startExpanded - ? TreeNodeState.EXPANDED - : TreeNodeState.COLLAPSED_WITH_JS; - nodesByIdRef.current.set(nodeId, existingNode); - } continue; } @@ -125,7 +119,7 @@ export function useIncrementalTreeData(items: ObjectItem[] | undefined, config: id: nodeId, item, parentId: nextParentId, - treeNodeState: TreeNodeState.LOADING, + treeNodeState: config.startExpanded ? TreeNodeState.EXPANDED : TreeNodeState.COLLAPSED_WITH_JS, title: nextTitle }; nodesByIdRef.current.set(nodeId, newNode); diff --git a/packages/pluggableWidgets/tree-node-web/src/components/v2/hooks/useInfiniteTreeNode.ts b/packages/pluggableWidgets/tree-node-web/src/components/v2/hooks/useInfiniteTreeNode.ts index 3d712423e2..daaea000b9 100644 --- a/packages/pluggableWidgets/tree-node-web/src/components/v2/hooks/useInfiniteTreeNode.ts +++ b/packages/pluggableWidgets/tree-node-web/src/components/v2/hooks/useInfiniteTreeNode.ts @@ -16,6 +16,15 @@ export function useInfiniteTreeNodes(props: TreeNodeContainerProps): { // loadedChilds : track the pre-loaded nodes of expanded nodes. const loadedChildsByIdRef = useRef>(new Map()); const initializedRef = useRef(false); + // Used only when startExpanded is false (only roots auto-expand; deeper tiers resolve via a + // real click through appendItems). Round 1 (pre-existing): preload roots' children, gated on + // content (loadedParentsByIdRef actually being populated), not on fire-count — so it retries + // harmlessly while the datasource is still empty/loading, and only locks in once real data + // lands. Round 2: once roots' children genuinely arrive, preload one level further for them + // too — same content-based gating, so it can't burn its one shot on a transient empty + // delivery before the real children show up. + const round1DoneRef = useRef(false); + const round2DoneRef = useRef(false); const getDatasourceFilter = useCallback( (items?: ItemType) => { @@ -38,25 +47,23 @@ export function useInfiniteTreeNodes(props: TreeNodeContainerProps): { (newItem: ObjectItem, children?: ObjectItem[]) => { const parentId = getItemId(newItem); - if (loadedParentsByIdRef.current.has(parentId)) { - if (children && children.length > 0) { - children.forEach(child => { - const childId = getItemId(child); - // get all expanded node's children Id, in order to pre-load them - // this is needed to be able to know if a node has further level children before expanding it. - loadedChildsByIdRef.current.set(childId, child); - }); + if (children && children.length > 0) { + children.forEach(child => { + const childId = getItemId(child); + // get all expanded node's children Id, in order to pre-load them + // this is needed to be able to know if a node has further level children before expanding it. + // Runs on every expand, including the first one — a node's own children being + // preloaded as part of its parent's expand must not delay preloading its grandchildren too. + loadedChildsByIdRef.current.set(childId, child); + }); + } - // if the new item is already in loadedChilds, - // it means that it was pre-loaded as a child of an expanded node, - // so we need to move it to loadedParents - if (loadedChildsByIdRef.current.has(parentId)) { - loadedParentsByIdRef.current.set(parentId, loadedChildsByIdRef.current.get(parentId)!); - loadedChildsByIdRef.current.delete(parentId); - } else { - loadedParentsByIdRef.current.set(parentId, newItem); - } - } + // if the new item is already in loadedChilds, + // it means that it was pre-loaded as a child of an expanded node, + // so we need to move it to loadedParents + if (loadedChildsByIdRef.current.has(parentId)) { + loadedParentsByIdRef.current.set(parentId, loadedChildsByIdRef.current.get(parentId)!); + loadedChildsByIdRef.current.delete(parentId); } else { loadedParentsByIdRef.current.set(parentId, newItem); } @@ -68,14 +75,60 @@ export function useInfiniteTreeNodes(props: TreeNodeContainerProps): { useEffect(() => { if (initializedRef.current) { - // after the first load of the datasource, - // we want to pre-load the child nodes of roots - if (loadedParentsByIdRef.current.size === 0) { + if (startExpanded) { + // Every level defaults to EXPANDED under "Start expanded" = Yes (not just roots), + // so keep treating newly-arrived items as loaded-parents and fetching their + // children, for as long as new descendants keep appearing. Self-terminating: + // once a round finds nothing new, it stops calling setFilter — bounded by the + // tree's real depth, not an arbitrary count. + let addedAny = false; datasource.items?.forEach(item => { - const parentId = getItemId(item); - loadedParentsByIdRef.current.set(parentId, item); + const id = getItemId(item); + if (!loadedParentsByIdRef.current.has(id)) { + loadedParentsByIdRef.current.set(id, item); + addedAny = true; + } }); + if (addedAny) { + datasource.setFilter(getDatasourceFilter(getExpandedFilterItems())); + } + return; + } + + if (!round1DoneRef.current) { + // after the first load of the datasource, + // we want to pre-load the child nodes of roots + if (loadedParentsByIdRef.current.size === 0) { + datasource.items?.forEach(item => { + const parentId = getItemId(item); + loadedParentsByIdRef.current.set(parentId, item); + }); + } + if (loadedParentsByIdRef.current.size > 0) { + round1DoneRef.current = true; + } datasource.setFilter(getDatasourceFilter(getExpandedFilterItems())); + return; + } + + if (!round2DoneRef.current) { + // Roots' children have arrived — preload one level further for them too, + // exactly like appendItems does for a manually expanded node, so their own + // expand affordance is known without an extra click. Only advances once real + // (not-yet-tracked) items are actually found, so it can't lock in prematurely + // on a transient empty/unchanged delivery. + let addedAny = false; + datasource.items?.forEach(item => { + const id = getItemId(item); + if (!loadedParentsByIdRef.current.has(id) && !loadedChildsByIdRef.current.has(id)) { + loadedChildsByIdRef.current.set(id, item); + addedAny = true; + } + }); + if (addedAny) { + round2DoneRef.current = true; + datasource.setFilter(getDatasourceFilter(getExpandedFilterItems())); + } } return; @@ -83,6 +136,8 @@ export function useInfiniteTreeNodes(props: TreeNodeContainerProps): { initializedRef.current = true; loadedParentsByIdRef.current.clear(); + round1DoneRef.current = false; + round2DoneRef.current = false; // when datasource is loaded for the first time, we want to load only the root nodes (nodes without parent) // if startExpanded is false, otherwise we want to load all nodes