From 4cf3b8d4e0d0571396ca78c9a5f34f57371161cd Mon Sep 17 00:00:00 2001 From: Petr Pokorny Date: Mon, 27 Jul 2026 09:42:46 +0200 Subject: [PATCH 1/3] Add MVU rules for DOM handle resolution and subscription keys Both rules generalise a defect found in the Overview Agents sticky strip, where observePinnedState captured .dashboard and its sentinel once inside a single requestAnimationFrame and never re-resolved them, under a subscription key that could not express the dependency. Copilot-Session: 96532087-8fa0-4bde-adca-56755c5d64f1 --- .github/instructions/client-mvu.instructions.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/.github/instructions/client-mvu.instructions.md b/.github/instructions/client-mvu.instructions.md index 2f441b5b..83df9e46 100644 --- a/.github/instructions/client-mvu.instructions.md +++ b/.github/instructions/client-mvu.instructions.md @@ -8,5 +8,7 @@ applyTo: "src/Client/**/*.fs" - Keep `update` pure: the same `(Msg, Model)` produces the same `(Model, Cmd)`. Capture clocks, randomness, and external results in commands or subscriptions and pass them through `Msg` payloads. - Event handlers dispatch messages for application behavior. They may also perform synchronous `preventDefault()`/`stopPropagation()` plumbing or pass rendering-only input to a bounded component-local hook. - Express API calls, timers, browser operations, and other application effects as `Cmd` or subscriptions. A component-local hook may schedule bounded browser work that only updates local presentation state and cleans up with the component lifecycle. +- Resolve DOM handles when an effect or subscription callback runs, not when it is attached. React can replace a node without any model change, so a captured element, observer root, or listener target detaches silently and never recovers. +- Key a subscription by every input its attached resource depends on, including the view structure it observes. A key that stays constant while its dependencies change keeps a dead subscription alive instead of restarting it. - Keep application state in `Model`; component-local hook state must remain presentation-only and must not communicate with `update`. - Use Feliz/React props and CSS classes instead of direct DOM mutation or inline style changes. From b897c49ae65510d61aa381c55a7e6e7414e36c90 Mon Sep 17 00:00:00 2001 From: Petr Pokorny Date: Mon, 27 Jul 2026 10:28:21 +0200 Subject: [PATCH 2/3] Distinguish per-invocation effects from node-bound resources The first rule was unimplementable for an observer or element listener: both must resolve their target at setup to attach at all, so re-querying inside their own callback cannot recover a replaced node. Split the guidance into resolving nodes per effect invocation and binding node-bound resources to a node React never replaces (or restarting them when the target can change). Copilot-Session: 96532087-8fa0-4bde-adca-56755c5d64f1 --- .github/instructions/client-mvu.instructions.md | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/.github/instructions/client-mvu.instructions.md b/.github/instructions/client-mvu.instructions.md index 83df9e46..be734eb0 100644 --- a/.github/instructions/client-mvu.instructions.md +++ b/.github/instructions/client-mvu.instructions.md @@ -8,7 +8,8 @@ applyTo: "src/Client/**/*.fs" - Keep `update` pure: the same `(Msg, Model)` produces the same `(Model, Cmd)`. Capture clocks, randomness, and external results in commands or subscriptions and pass them through `Msg` payloads. - Event handlers dispatch messages for application behavior. They may also perform synchronous `preventDefault()`/`stopPropagation()` plumbing or pass rendering-only input to a bounded component-local hook. - Express API calls, timers, browser operations, and other application effects as `Cmd` or subscriptions. A component-local hook may schedule bounded browser work that only updates local presentation state and cleans up with the component lifecycle. -- Resolve DOM handles when an effect or subscription callback runs, not when it is attached. React can replace a node without any model change, so a captured element, observer root, or listener target detaches silently and never recovers. +- Resolve DOM nodes when an effect runs, never at the point the effect is created. React can replace a node without any model change, so a captured element goes stale silently and nothing reports it. +- A resource that binds to a node at setup — an observer, or a listener on anything other than `document` — cannot recover by re-querying inside its own callback, because that callback stops firing meaningfully once its target is replaced. Attach it to a node React never replaces and resolve the volatile targets per event, or restart it whenever its target can change. - Key a subscription by every input its attached resource depends on, including the view structure it observes. A key that stays constant while its dependencies change keeps a dead subscription alive instead of restarting it. - Keep application state in `Model`; component-local hook state must remain presentation-only and must not communicate with `update`. - Use Feliz/React props and CSS classes instead of direct DOM mutation or inline style changes. From 8c4aad59f54924a3d6b39a98786c6a53d6591ccf Mon Sep 17 00:00:00 2001 From: Petr Pokorny Date: Fri, 31 Jul 2026 09:28:03 +0200 Subject: [PATCH 3/3] Keep the Overview pinned-state observers bound to the live dashboard The Agents strip stopped closing its drill-down on scroll and stopped scrolling to the top on a pinned click, because OverviewAgentsStuck was never dispatched again. Two independent causes: - .app-layout reordered its two unkeyed, same-typed children by dock position, so React reconciled them by index and the observers ended up watching the canvas pane whenever the canvas was docked left or top. Docking is now purely CSS (order), with DOM order fixed. - observePinnedState resolved its nodes in a single animation frame and gave up silently when that frame ran before React committed the band. It now re-resolves each frame until the nodes exist. Both observers stay: the IntersectionObserver keeps its initial callback and non-scroll geometry updates, and setCircleShift stays on the ResizeObserver rather than on the scroll path. Copilot-Session: 4d993231-8a87-4e1b-a1e3-bacde1d5db21 --- docs/spec/beads-overview-band.md | 5 +- docs/spec/canvas-pane.md | 4 + docs/spec/overview-drilldown.md | 4 +- src/Client/App.fs | 13 +-- src/Client/OverviewBand.fs | 18 +++- src/Client/index.html | 3 + src/Tests/OverviewBandE2ETests.fs | 151 ++++++++++++++++++++++++++++++ 7 files changed, 184 insertions(+), 14 deletions(-) diff --git a/docs/spec/beads-overview-band.md b/docs/spec/beads-overview-band.md index 14dc5e11..42276d55 100644 --- a/docs/spec/beads-overview-band.md +++ b/docs/spec/beads-overview-band.md @@ -279,7 +279,10 @@ the solution compiling (no compat shims, per house rules). geometry and refreshed by `ResizeObserver`, avoiding platform-font pixel tuning. An `IntersectionObserver` watches only the sentinel and reports pinned after it passes strictly above the dashboard boundary. Its Elmish subscription exists only while agent groups are rendered, so - removing the Agents DOM disposes the observers and resets pinned state. Entering the pinned state + removing the Agents DOM disposes the observers and resets pinned state. Attachment resolves its + nodes on each animation frame until they exist, because the subscription can start before React + has committed the band and a single missed lookup would otherwise leave the strip permanently + unpinned with no error. Entering the pinned state closes an agent drill-down and switches Agents to one `nowrap` row with hidden-scrollbar horizontal overflow. Expanded category columns still wrap normally. diff --git a/docs/spec/canvas-pane.md b/docs/spec/canvas-pane.md index 2e10381c..e08f16be 100644 --- a/docs/spec/canvas-pane.md +++ b/docs/spec/canvas-pane.md @@ -58,6 +58,10 @@ A `SystemView` drives its own updates, so it needs neither morph nor the author - The pane opens and closes from the header Canvas button and the `C` key. - Open or closed state persists in global config. - Position selector supports left, right, top, and bottom docking, and the selected position persists. + `.app-layout` always renders the dashboard first and the pane second; docking is purely a CSS + concern (`flex-direction` plus `order` keyed off the layout's position class). Reordering the two + same-typed children in the DOM instead would make React reconcile them by index, silently + re-rendering each existing node with the other subtree and stranding anything bound to those nodes. - Size selector supports 1:1 (default) and 2:1 — at 2:1 the open pane takes two-thirds of the layout instead of half — and the selected size persists in global config. - The pane normally follows the focused worktree. An explicit card-level SystemView action may target another worktree without moving dashboard card focus; the next explicit card selection clears that override. - The worktree diff is explicit-only when another canvas document exists. Automatic fallback and explicit card selection prefer another document; `diff.html` is selected automatically only when it is the worktree's sole canvas document. The card Diff action and direct tab selection still open it. The server omits the generated `diff.html` from a confirmed-clean worktree's inventory (`docs/spec/worktree-diff-viewer.md`), so a clean worktree shows no diff tab — the tab strip needs no per-view visibility rule of its own. diff --git a/docs/spec/overview-drilldown.md b/docs/spec/overview-drilldown.md index e631e2d0..f98387ce 100644 --- a/docs/spec/overview-drilldown.md +++ b/docs/spec/overview-drilldown.md @@ -157,7 +157,9 @@ type OverviewSelection = translation is derived from rendered geometry and refreshed by `ResizeObserver`, so platform font metrics land on the same center. An `IntersectionObserver` is active only while agent groups exist, watches only the sentinel, closes an agent selection after it passes strictly above the - dashboard boundary, and enables the pinned `nowrap` layout with horizontal overflow. + dashboard boundary, and enables the pinned `nowrap` layout with horizontal overflow. It re-resolves + its nodes each frame until the band is committed, so a subscription that starts ahead of React's + commit still attaches. - Render the breakdown panel below the relevant row when a matching group is selected: the ✕ close button (top-right corner, absolutely positioned so it adds no vertical space), repo-grouped members, agent chips vs. task bars. diff --git a/src/Client/App.fs b/src/Client/App.fs index 545a1423..16ddece9 100644 --- a/src/Client/App.fs +++ b/src/Client/App.fs @@ -1076,18 +1076,15 @@ let view model dispatch = let canvasEl = CanvasView.view model dispatch - let children = - match model.Canvas.CanvasPosition with - | CanvasPosition.Left - | CanvasPosition.Top -> [ canvasEl; dashboardEl ] - | CanvasPosition.Right - | CanvasPosition.Bottom -> [ dashboardEl; canvasEl ] - + // DOM order is fixed; the dock position is applied by CSS `order` on .app-layout's position + // class. Reordering these two same-typed divs instead makes React reconcile them by index, which + // silently re-renders each existing node with the other subtree and leaves node-bound resources + // (the Overview sticky observers) watching the wrong pane. React.Fragment [ viewAppHeader model dispatch Html.div [ prop.className layoutClass - prop.children children + prop.children [ dashboardEl; canvasEl ] ] ] diff --git a/src/Client/OverviewBand.fs b/src/Client/OverviewBand.fs index d395a480..2df4c45f 100644 --- a/src/Client/OverviewBand.fs +++ b/src/Client/OverviewBand.fs @@ -81,11 +81,21 @@ let private createPinnedObservers (onChange: bool -> unit) = | _ -> [] let observePinnedState (onChange: bool -> unit) = - // Observer attachment follows the React commit, so the handles must live across the frame callback. + // The band's nodes only exist once React has committed, which can happen after this + // subscription starts, so re-resolve them every frame until they appear rather than giving up + // silently on the first miss. Mutation is the impure boundary: the frame callback runs after + // this function returns, yet Dispose has to reach whatever it eventually attached. let mutable observers = [] - let frameId: int = - Dom.window?requestAnimationFrame(fun (_: float) -> - observers <- createPinnedObservers onChange) + let mutable frameId = 0 + + let rec attachOnNextFrame () = + frameId <- + Dom.window?requestAnimationFrame(fun (_: float) -> + match createPinnedObservers onChange with + | [] -> attachOnNextFrame () + | attached -> observers <- attached) + + attachOnNextFrame () { new System.IDisposable with member _.Dispose() = diff --git a/src/Client/index.html b/src/Client/index.html index 730a8277..074abb9c 100644 --- a/src/Client/index.html +++ b/src/Client/index.html @@ -764,6 +764,9 @@ .app-layout.canvas-size-2to1.canvas-bottom > .canvas-pane.open { flex: 0 0 66.667%; } + /* Docking left/top paints the pane first without changing DOM order (see App.fs view). */ + .app-layout.canvas-left > .canvas-pane, + .app-layout.canvas-top > .canvas-pane { order: -1; } .app-layout.canvas-right > .canvas-pane { border-left: 1px solid #313244; } .app-layout.canvas-left > .canvas-pane { border-right: 1px solid #313244; } .app-layout.canvas-top > .canvas-pane { border-bottom: 1px solid #313244; } diff --git a/src/Tests/OverviewBandE2ETests.fs b/src/Tests/OverviewBandE2ETests.fs index 4be81e62..179629d9 100644 --- a/src/Tests/OverviewBandE2ETests.fs +++ b/src/Tests/OverviewBandE2ETests.fs @@ -167,6 +167,16 @@ let private bandProbeJs = } """ +/// Scrolls the dashboard just past the sticky boundary, where the band is fully pinned. +let private scrollPastStickyBoundaryJs = + """ + () => { + const dashboard = document.querySelector('.dashboard'); + const morphRange = parseFloat(getComputedStyle(dashboard).getPropertyValue('--overview-agents-morph-range')); + dashboard.scrollTop = morphRange + 1; + } + """ + let private cardProbeJs = """ () => { @@ -993,3 +1003,144 @@ type OverviewBandE2ETests() = PageWaitForFunctionOptions(Timeout = 5000.0f)) () } + + // Docking left/top used to reverse the two .app-layout children, which React reconciled by + // index — the pinned-state observers silently ended up watching the canvas pane, so neither + // scroll-closes-drill-down nor pinned-click-scrolls-to-top fired again. + [] + member this.``Docking the canvas left keeps pinned-state detection alive``() = + task { + let canvasBtn = + this.Page.Locator(".header-controls .ctrl-btn", PageLocatorOptions(HasText = "Canvas")) + do! canvasBtn.ClickAsync() + do! this.Page.Locator(".canvas-tab-bar").WaitForAsync(LocatorWaitForOptions(Timeout = 5000.0f)) + do! this.Page.Locator(".canvas-pos-btn[title='Dock left']").ClickAsync() + do! this.Page.Locator(".app-layout.canvas-left").WaitForAsync(LocatorWaitForOptions(Timeout = 5000.0f)) + + let! layoutJson = + this.Page.EvaluateAsync( + """() => { + const layout = document.querySelector('.app-layout'); + const dashboard = document.querySelector('.dashboard'); + const pane = document.querySelector('.canvas-pane'); + return JSON.stringify({ + firstChildIsDashboard: layout.firstElementChild === dashboard, + panePaintsLeft: pane.getBoundingClientRect().left < dashboard.getBoundingClientRect().left + }); + }""") + + let investigating = + this.Page.Locator(".overview-agents-band .overview-item", PageLocatorOptions(HasText = "Investigating")) + do! investigating.ClickAsync() + do! this.Page.Locator(".overview-breakdown").WaitForAsync(LocatorWaitForOptions(Timeout = 5000.0f)) + + let! _ = this.Page.EvaluateAsync(scrollPastStickyBoundaryJs) + + let! _ = + this.Page.WaitForFunctionAsync( + """() => !document.querySelector('.overview-breakdown') + && !document.querySelector('.overview-item-selected')""", + null, + PageWaitForFunctionOptions(Timeout = 5000.0f)) + + do! investigating.Locator(".overview-circle").First.ClickAsync() + let! _ = + this.Page.WaitForFunctionAsync( + """() => document.querySelector('.dashboard').scrollTop <= 0.5 + && !!document.querySelector('.overview-breakdown')""", + null, + PageWaitForFunctionOptions(Timeout = 5000.0f)) + + let layout = JObject.Parse(layoutJson) + Assert.That(layout.Value("firstChildIsDashboard"), Is.True, "the dashboard stays layout child #0 in every dock position") + Assert.That(layout.Value("panePaintsLeft"), Is.True, "canvas-left paints the pane left of the dashboard via CSS order") + } + + // A first attach that lands before React has committed the band must not give up silently: the + // sentinel is withheld from the first few lookups here, standing in for the real commit race + // that leaves the strip glued from page load. + [] + member this.``Pinned-state observers retry until the band is committed``() = + task { + do! + this.Page.AddInitScriptAsync( + """ + window.__sentinelMissBudget = 3; + const original = Document.prototype.querySelector; + Document.prototype.querySelector = function (selector) { + if (selector === '.overview-agents-stick-sentinel' && window.__sentinelMissBudget > 0) { + window.__sentinelMissBudget--; + return null; + } + return original.call(this, selector); + }; + """) + + let! _ = this.Page.ReloadAsync() + do! this.Page.Locator(".wt-card .branch-name").First.WaitForAsync(LocatorWaitForOptions(Timeout = 15000.0f)) + + let overviewBtn = + this.Page.Locator(".header-controls .ctrl-btn", PageLocatorOptions(HasText = "Overview")) + do! overviewBtn.ClickAsync() + do! this.Page.Locator(".overview-agents-band").WaitForAsync(LocatorWaitForOptions(Timeout = 5000.0f)) + + let! _ = + this.Page.WaitForFunctionAsync( + "() => window.__sentinelMissBudget === 0", + null, + PageWaitForFunctionOptions(Timeout = 5000.0f)) + + do! + this.Page + .Locator(".overview-agents-band .overview-item", PageLocatorOptions(HasText = "Investigating")) + .ClickAsync() + do! this.Page.Locator(".overview-breakdown").WaitForAsync(LocatorWaitForOptions(Timeout = 5000.0f)) + + let! _ = this.Page.EvaluateAsync(scrollPastStickyBoundaryJs) + + let! _ = + this.Page.WaitForFunctionAsync( + """() => !document.querySelector('.overview-breakdown') + && !document.querySelector('.overview-item-selected')""", + null, + PageWaitForFunctionOptions(Timeout = 5000.0f)) + () + } + + // Opening the band below the sticky boundary must report the pinned state on attach, with no + // scroll to trigger it — the observers resolve their nodes after the React commit, not before. + [] + member this.``Overview opened while the dashboard is scrolled starts pinned``() = + task { + let overviewBtn = + this.Page.Locator(".header-controls .ctrl-btn", PageLocatorOptions(HasText = "Overview")) + do! overviewBtn.ClickAsync() + do! Assertions.Expect(this.Page.Locator(".overview-agents-band")).ToHaveCountAsync(0) + + let! scrolled = + this.Page.EvaluateAsync( + """() => { + const dashboard = document.querySelector('.dashboard'); + dashboard.scrollTop = dashboard.scrollHeight; + return dashboard.scrollTop; + }""") + + Assert.That(scrolled, Is.GreaterThan(150.0), "the dashboard must be scrollable for this scenario to mean anything") + + do! overviewBtn.ClickAsync() + do! this.Page.Locator(".overview-agents-band").WaitForAsync(LocatorWaitForOptions(Timeout = 5000.0f)) + + do! + this.Page + .Locator(".overview-agents-band .overview-item", PageLocatorOptions(HasText = "Investigating")) + .Locator(".overview-circle") + .First.ClickAsync() + + let! _ = + this.Page.WaitForFunctionAsync( + """() => document.querySelector('.dashboard').scrollTop <= 0.5 + && !!document.querySelector('.overview-breakdown')""", + null, + PageWaitForFunctionOptions(Timeout = 5000.0f)) + () + }