Skip to content

Run JSX listeners deepest-first by binding them to their own node - #2

Open
SantosNr22 wants to merge 1 commit into
geastack:mainfrom
SantosNr22:fix/jsx-listener-bubble-order
Open

SantosNr22 wants to merge 1 commit into
geastack:mainfrom
SantosNr22:fix/jsx-listener-bubble-order

Conversation

@SantosNr22

@SantosNr22 SantosNr22 commented Oct 1, 2026 •

Copy link
Copy Markdown

Problem

On the native (macOS) target, when a keyed list row and one of its ancestors both have
onPointerDown (or any other bubbling listener), the order in which they run flips after the
list has re-rendered once:

press before first re-render after a re-render
order row, then ancestor (IA) ancestor, then row (AI)

Downstream effect (Daymark week grid): a block's onPointerDown guards on mode === "none" and the
grid's onPointerDown starts a "create" gesture. After the first gesture the grid ran first, set
mode = "create", and the block's handler returned at its guard. It looked like "the block's handler
never fires"; it fires, but second. stopPropagation() has no effect between handlers on different
nodes for the same reason.

The web target is not affected (see "Web target" below).

Cause

bindNodeListener registered every bubbling listener on the document body, gated by
Tree::containsNode(target, event.targetId). Tree::dispatchEvent runs the listeners of one node in
registration order, so the body ran nested handlers in the order they were bound, not deepest first.

At mount, a list's rows are built (reactiveListApply calls the row thunk once) before the list
container's own onPointerDown is bound, so the row is bound first and the order happens to be right.
On every later re-render the rows are rebuilt and bound after the ancestors, so they run after them.

The comments above addNodeListener say the engine "does not walk ancestors". That is not true of
any published @geastack/engine (0.1.5, 0.1.6, 0.1.7 all start at event.targetId and walk
parent, honour event.bubbles and propagationStopped), so body delegation is no longer needed
for bubbling.

Fix

Register the listener on the node itself (what the scroll branch already does), for every event that
is not document-level. Tree::dispatchEvent then does the bubbling: descendants first, then
ancestors; stopPropagation stops the walk; non-bubbling events stay on their target. The engine drops
a node's listeners when the node is released (releaseRareData, also when its id is reused by
createNode), so a rebuilt row never leaves a stale generation of its handler behind, which is what the
old body-delegation comment was protecting against.

The scroll special case and the recordNodeSubscription release for body listeners become
unnecessary and are removed; comments updated to match.

Reproduction

The reproduction app's source is inlined at the bottom of this description. A ReactiveComponent with an
ancestor onPointerDown, keyed rows with their own onPointerDown, and a release handler that
reassigns this.items. The status line prints order.

Build and run: npx gea build --target macos, then open dist/macos/listrepro/Listrepro.app, and click
Item A three times with a real mouse (CGEvents; synthetic NSEvents skip the gesture recognizer).

build order after 3 presses
unpatched IAAIAI (press 1 correct, presses 2 and 3 reversed)
patched IAIAIA (every press row-then-ancestor), itemDowns=3 ancestorDowns=3

itemDowns=3 after 3 presses on the patched build also shows a rebuilt row runs its handler once, not
once per generation.

Second check, the actual Daymark week-grid spike (blocks with a nested resize handle, drag to move,
drag empty grid to create), workaround removed, real-mouse drags, same pinned install:

press 1 press 2 drag 1 drag 2
unpatched block grid creates "New block" creates "New block"; s1 never moves
patched block block s1 09:00 to 09:30 s1 to 10:00; dragging the nested handle resizes 10:00-11:30 to 10:00-12:00

Versions

@geastack/apple 0.2.11 (re-checked on 0.2.12, below), core 0.1.28, cli 0.1.86, targets 0.1.83, engine 0.1.6,
compiler 1.0.18 (exact npm ci from the spike's lockfile), macOS 26 (Darwin 25.6), Apple Silicon.
The fix was checked two ways: by applying the same edit to the 1.0.18 header in a pristine npm ci
install, and by building the repro against this branch's gea_runtime.h.

Re-checked against newer releases

@geastack/apple 0.2.12 (changes macOS hit-testing: text labels now participate in hit tests, and the
root press bridge hit-tests in the root's own coordinate space) does not change the ordering bug.
Same repro, same real-mouse protocol, everything else at the pins above:

apple 0.2.12 + order after 3 presses
compiler 1.0.18 header, unpatched IAAIAI (bug persists)
this branch's gea_runtime.h IAIAIA

@geastack/compiler 1.0.19 and origin/main (no commits beyond the base of this branch) still
delegate listeners to the body, so the patch applies unchanged. The week-grid drag run was not repeated
on 0.2.12. The press target can now be a text node inside a block; the fix relies on the engine walking
parents, so that case bubbles correctly, but I only exercised it through the repro's <span> rows.

Web target

Not affected, but for a different reason worth knowing: the web runtime installs one document
listener per event type and calls only the nearest node that has a handler, then returns. So on web
the row handler runs and the ancestor's does not run at all (order=III, ancestorDowns=0; the
ancestor fires only for a press that lands on it directly). After this change the native target
bubbles (row, then ancestor), which is what event.bubbles/stopPropagation describe but is not what
web does. I did not change web; flagging it because app code that relies on either behavior will
differ between targets.

How it was verified, and what was not

Ran:

  • Real-mouse repro on the pristine, version-pinned install: unpatched shows the flip, patched does not.
  • Same for the week-grid spike (table above), including the nested resize handle.
  • The patched HEAD gea_runtime.h builds and behaves the same in the repro app.
  • node scripts/check-runtime-header.mjs (both wchar widths) and npx tsc -p tsconfig.json --noEmit
    pass on the branch.
  • The web build of the repro in a browser (no flip; ancestor never runs, as above).

Not verified / limits:

  • No automated regression test. This repo has no runner that links the engine: JSX fixtures are
    compile-only and pin emitted C++, and this change is in the runtime header, so a fixture cannot
    fail on this bug. check-runtime-header.mjs compiles the header without GEA_HOST_DECLARED, so it does
    not compile the changed code either (the app builds above do). A test that dispatches
    Tree::dispatchEvent over a row rebuilt after its ancestor needs the engine, e.g. beside
    packages/core/test/test_gea_engine_rare_data_main.cpp in geastack/core.
  • npm run gate fails on this machine with 166 programs "moved" on unmodified HEAD too; the set is
    identical with and without this commit, so I cannot show byte-identical emitted output from the
    gate, only that it does not change what the gate reports. Baselines were not re-taken.
  • Only macOS was run. Other native targets (iOS, Windows, ESP32, GeaOS) use the same header and the same
    runtime.cpp -> Tree::dispatchEvent path, but I did not build them.
  • keydown/input/click listeners take the same per-node path; I exercised pointer events only.
  • Not run: the rest of npm test, scripts/run-runtime-tests.mjs.
  • Side note (unrelated): a fresh npm install of the spike resolves engine 0.1.7 and fails to build
    (elements/ui/virtual_list.cpp: itemCount on a unique_ptr), so elements 0.1.2 and engine 0.1.7
    do not match. The lockfile pins avoid it.

Repro source (pr-repro/src/Repro.tsx)

import { ReactiveComponent, type PointerEvent } from "@geastack/core";

interface Item {
  id: string;
  label: string;
  top: number;
}

function makeItems(shift: number): Item[] {
  return [
    { id: "a", label: `Item A ${shift}`, top: 0 + shift },
    { id: "b", label: `Item B ${shift}`, top: 48 + shift },
    { id: "c", label: `Item C ${shift}`, top: 96 + shift },
  ];
}

// Reproduction: keyed list rows with their own onPointerDown inside an ancestor
// that also has onPointerDown. Releasing a press reassigns this.items, which
// re-renders the rows. `order` records which handler ran first on each press:
// "IA" = row then ancestor (correct), "AI" = ancestor then row (the bug).
export class Repro extends ReactiveComponent {
  items: Item[] = makeItems(0);
  shift = 0;
  pressed = "";
  itemDowns = 0;
  ancestorDowns = 0;
  rerenders = 0;
  lastItem = "none";
  order = "";

  itemDown(id: string, event: PointerEvent) {
    this.itemDowns = this.itemDowns + 1;
    this.order = `${this.order}I`;
    this.lastItem = id;
    this.pressed = id;
  }

  ancestorDown(event: PointerEvent) {
    this.ancestorDowns = this.ancestorDowns + 1;
    this.order = `${this.order}A`;
  }

  rerender(event: PointerEvent) {
    this.rerenders = this.rerenders + 1;
    this.items = makeItems(this.shift);
  }

  // Like a drag gesture: release anywhere re-renders the list with changed data.
  release(event: PointerEvent) {
    if (this.pressed === "") return;
    this.pressed = "";
    this.shift = this.shift + 4;
    this.rerenders = this.rerenders + 1;
    this.items = makeItems(this.shift);
  }

  template() {
    return (
      <div class="screen" onPointerUp={(event: PointerEvent) => this.release(event)}>
        <div class="list" style={{ position: "relative", height: "160px" }} onPointerDown={(event: PointerEvent) => this.ancestorDown(event)}>
          {this.items.map((item) => (
            <div key={item.id} class={this.pressed === item.id ? "item active" : "item"} style={{ position: "absolute", left: "8px", right: "8px", top: `${item.top}px` }} onPointerDown={(event: PointerEvent) => this.itemDown(item.id, event)}>
              <span>{item.label}</span>
            </div>
          ))}
        </div>
        <div class="button" onPointerDown={(event: PointerEvent) => this.rerender(event)}>
          <span>Re-render list</span>
        </div>
        <span class="status">{`itemDowns=${this.itemDowns} ancestorDowns=${this.ancestorDowns} rerenders=${this.rerenders} last=${this.lastItem} order=${this.order}`}</span>
      </div>
    );
  }
}

Summary by CodeRabbit

  • Improvements
    • Event listeners are now registered directly on their target elements for all event types, including scroll events. This makes event handling consistent across event types and ties each listener to the element it serves. When an element is removed, its associated listener no longer relies on a page-level event listener.

bindNodeListener delegated every bubbling JSX listener to the document
body, gated by containsNode. The engine runs a node's listeners in
registration order, so the body ran nested handlers in the order they
were bound, not deepest first. A list row rebuilt by a re-render binds
after the ancestors that were bound once at mount, so with an
onPointerDown on both the row and an ancestor the order flipped from
row-then-ancestor to ancestor-then-row after the first re-render, and
stopPropagation could not hold the ancestor back.

Register the listener on the node itself, as scroll already is. The
engine's Tree::dispatchEvent walks from the hit node up its parents, so
descendants run first, stopPropagation and non-bubbling events work, and
the listeners are released with the node (releaseRareData).

Runtime-header change only. The emitted-set gate names the same moved
programs with and without it (its baselines do not reproduce on the
machine this was written on, so they are not re-taken here).

Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
@github-actions

github-actions Bot commented Oct 1, 2026

Copy link
Copy Markdown

Thanks for the pull request. Before it can be merged, please read the GeaStack Contributor License Agreement and sign it by posting a comment here with exactly:


I have read the CLA Document and I hereby sign the CLA


You can retrigger this bot by commenting recheck in this Pull Request. Posted by the CLA Assistant Lite bot.

@coderabbitai

coderabbitai Bot commented Oct 1, 2026 •

Copy link
Copy Markdown

Review in Change Stack →

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

🧰 Additional context used
📚 Code guidelines (1)
CLAUDE.md — auto-discovered

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Advanced

Run ID: 92425a59-c910-433c-9a09-78c0c34a2560

📥 Commits

Reviewing files that changed from the base of the PR and between a7c4cf8 and ec13908.

📒 Files selected for processing (1)
  • src/targets/cpp/runtime/gea_runtime.h

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 9 remain after this review.


📝 Walkthrough

Walkthrough

bindNodeListener now registers listeners directly on the target node for all event names. Related comments describe node-based registration, ancestor dispatch, and listener cleanup when a node is taken down.

Changes

Node listeners

Layer / File(s) Summary
Direct node registration and listener documentation
src/targets/cpp/runtime/gea_runtime.h
bindNodeListener registers listeners on the target node for all event names. The comments describe node registration, ancestor dispatch, handler state, and cleanup on node teardown.

Priority: ⬇️ Low

Estimated code review effort: 2 (Simple) | ~10 minutes

Change: Bug fix

Suggested reviewers: dashersw

Merge Risk: ⚪ Minimal · up to ec139

Listeners now register on their target nodes. Available evidence does not establish a regression, though the external engine’s propagation and teardown behavior remains unverified and should be confirmed in supported native testing.

Architecture Summary

Architecture risk: 🔵 Low · up to ec139

The change affects 1 system.

Changed systems: src

Architecture concerns
No architecture-level concerns identified.

Review details

Systems and components

  • observed — src (service) was modified; 1 changed file maps to changed impact.

Before / after behavior

  • observed — Modified behavior in src/targets/cpp/runtime/gea_runtime.h: The comment now describes node-owned listeners and ancestor dispatch, replacing the description of BODY delegation and its containsNode filter.
  • observed — Modified behavior in src/targets/cpp/runtime/gea_runtime.h: bindNodeListener now registers the listener on the target for all event names. Previously, only scroll used the target; other events were registered on the BODY, filtered by containsNode, and removed through a node subscription.
  • observed — Modified behavior in src/targets/cpp/runtime/gea_runtime.h: The comment for the no-argument addNodeListener overload now describes node registration, ancestor traversal, handler target and phase state, and listener cleanup on node teardown. It replaces the description of BODY delegation and stale handlers from rebuilt nodes.
  • observed — Modified behavior in src/targets/cpp/runtime/gea_runtime.h: The comment for the event-argument overload now describes node registration and ancestor dispatch, replacing the description of BODY delegation and the prior claim that the wrapper did not receive events targeted at its children.
🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and concisely describes the main change: binding JSX listeners to their own nodes so nested listeners run deepest-first.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 0…
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches 💡 1
🛠️ Fix failing CI checks 💡
  • Commit to this branch
  • Create a new PR
🧪 Generate unit tests (beta)
  • Create a new PR
  • Autopilot · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Autopilot is currently an internal CodeRabbit preview.

Warning

Some tools did not complete. Review the errors below.

🔧 ast-grep (0.45.3)
src/targets/cpp/runtime/gea_runtime.h

ast-grep timed out on this file


Comment @coderabbitai help to get the list of available commands.

@SantosNr22

Copy link
Copy Markdown
Author

I have read the CLA Document and I hereby sign the CLA

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant