Merge pull request #72 from coasys/feat/query-abort

jhweir · web-flow · commit 179e746e291c · 2026-06-17T19:29:44.000+01:00
feat(schema-renderer): abort in-flight findAll on cleanup and re-run
diff --git a/.github/workflows/build.yaml b/.github/workflows/build.yaml
@@ -0,0 +1,128 @@
+name: Build
+
+on:
+  push:
+    branches: [dev]
+  pull_request:
+
+jobs:
+  build:
+    runs-on: ubuntu-latest
+    steps:
+      - uses: actions/checkout@v4
+
+      # WE's .nvmrc pins Node 24 (uses Node 22+ features like `fs.globSync`).
+      - uses: actions/setup-node@v4
+        with:
+          node-version: "24"
+
+      # ad4m's monorepo uses object-format pnpm.overrides which pnpm v10
+      # rejects.  Install pnpm 9.15.0 directly via npm and pin it for the
+      # whole workflow — WE's `packageManager: pnpm@10.18.3` works fine
+      # under pnpm 9 (the warning is cosmetic) and pnpm/action-setup@v4
+      # refuses to override packageManager without erroring.
+      - name: Install pnpm 9.15.0
+        run: npm install -g pnpm@9.15.0
+
+      - name: Detect branch
+        id: branch
+        run: |
+          BRANCH="${GITHUB_HEAD_REF:-${GITHUB_REF#refs/heads/}}"
+          echo "name=$BRANCH" >> "$GITHUB_OUTPUT"
+          if git ls-remote --exit-code --heads \
+            https://github.com/coasys/ad4m.git "$BRANCH" >/dev/null 2>&1; then
+            echo "ad4m_branch=$BRANCH" >> "$GITHUB_OUTPUT"
+            echo "✅ Found matching AD4M branch: $BRANCH"
+          else
+            echo "ad4m_branch=dev" >> "$GITHUB_OUTPUT"
+            echo "ℹ️ No matching AD4M branch — using dev"
+          fi
+
+      # --- AD4M linked build (published packages have broken workspace: refs) ---
+
+      - name: Clone AD4M
+        run: |
+          git clone --depth 1 --single-branch \
+            --branch "${{ steps.branch.outputs.ad4m_branch }}" \
+            https://github.com/coasys/ad4m.git ad4m
+
+      - name: Get AD4M commit hash
+        id: ad4m-hash
+        run: |
+          HASH=$(cd ad4m && git rev-parse HEAD)
+          echo "hash=$HASH" >> "$GITHUB_OUTPUT"
+          echo "AD4M commit: $HASH"
+
+      - name: Cache AD4M built outputs
+        id: ad4m-cache
+        uses: actions/cache@v4
+        with:
+          path: |
+            ad4m/node_modules
+            ad4m/core/lib
+            ad4m/connect/dist
+          key: ad4m-sdk-${{ steps.ad4m-hash.outputs.hash }}
+
+      - name: Install & build AD4M packages
+        if: steps.ad4m-cache.outputs.cache-hit != 'true'
+        run: |
+          cd ad4m
+          pnpm install --no-frozen-lockfile
+
+          cd core && pnpm exec tsc && pnpm run bundle && cd ..
+          cd connect && pnpm run build && cd ..
+
+      # --- WE build ---
+
+      - name: Cache WE dependencies
+        uses: actions/cache@v4
+        with:
+          path: |
+            node_modules
+            */node_modules
+            !ad4m/node_modules
+          key: we-deps-${{ hashFiles('pnpm-lock.yaml') }}
+
+      - name: Override AD4M packages with local builds
+        run: |
+          node -e "
+            const pkg = require('./package.json');
+            pkg.pnpm = pkg.pnpm || {};
+            pkg.pnpm.overrides = pkg.pnpm.overrides || {};
+            pkg.pnpm.overrides['@coasys/ad4m'] = 'file:./ad4m/core';
+            pkg.pnpm.overrides['@coasys/ad4m-connect'] = 'file:./ad4m/connect';
+            require('fs').writeFileSync('./package.json', JSON.stringify(pkg, null, 2) + '\n');
+          "
+
+      - name: Install WE dependencies
+        run: pnpm install --no-frozen-lockfile
+
+      - name: Clear build caches for linked SDK
+        run: |
+          rm -rf .turbo node_modules/.cache
+          find . -name '.turbo' -type d -not -path './ad4m/*' -not -path './node_modules/*' -exec rm -rf {} + 2>/dev/null || true
+          find . -name '.vite' -type d -path '*/node_modules/.vite' -not -path './ad4m/*' -exec rm -rf {} + 2>/dev/null || true
+
+      # Non-blocking: this is the first CI workflow on the WE repo, so
+      # pre-existing lint debt on `dev` (mostly `no-explicit-any`
+      # warnings, plus the occasional unsorted import) shows up the
+      # moment lint actually runs.  Surface the warnings to reviewers
+      # via the run annotations, but don't fail the build until the
+      # backlog is cleared in a follow-up.
+      - name: Lint
+        run: NODE_OPTIONS='--max-old-space-size=8192' pnpm lint
+        continue-on-error: true
+
+      - name: Build
+        run: NODE_OPTIONS='--max-old-space-size=8192' pnpm build
+
+      # Non-blocking on first CI introduction.  Same rationale as Lint:
+      # `dev` carries pre-existing failures in unrelated packages (e.g.
+      # an app-framework test imports a file that no longer exists) and
+      # this PR is the first to actually run vitest in CI.  Surface the
+      # failures in the run summary so a follow-up can clear the
+      # backlog, but don't block the abort-controller work on unrelated
+      # test debt.
+      - name: Test
+        run: pnpm test
+        continue-on-error: true
diff --git a/eslint.config.js b/eslint.config.js
@@ -17,6 +17,11 @@ export default [
       '**/target/**',
       '**/dist-electron/**',
       '**/.storybook/**',
+      // The branch-aware ad4m build (scripts/build-with-ad4m-link.sh) and
+      // its CI counterpart clone the matching ad4m branch into ./ad4m and
+      // link it via pnpm.overrides.  ESLint must not walk into that
+      // checkout — it's not WE source, and walking it OOMs the linter.
+      'ad4m/**',
     ],
   },
   {
diff --git a/packages/schema-system/frameworks/solid/src/SchemaRenderer.tsx b/packages/schema-system/frameworks/solid/src/SchemaRenderer.tsx
@@ -242,9 +242,29 @@ function createQuerySignal(
         });
       onCleanup(() => builder.dispose());
     } else {
-      (ModelClass.findAll(p, queryOptions) as Promise<unknown[]>).then((results) => {
-        setItems(reconcile(normalise(results), { key: 'id', merge: true }));
-      });
+      // The effect re-runs when any reactive dep changes — perspective swap,
+      // resolved params, etc.  When that happens (or the component unmounts)
+      // the previous findAll may still be in flight against a slow query.
+      // An AbortController scoped to this iteration tells the executor to
+      // drop the JSON reply for the stale query instead of paying its
+      // serialise + WebSocket + deserialise cost.
+      //
+      // ad4m's `Ad4mModel.findAll` accepts `options?: { signal?: AbortSignal }`
+      // as the 3rd argument and forwards it through `perspective.modelQuery`
+      // to the executor's `request.cancel` machinery.
+      const controller = new AbortController();
+      onCleanup(() => controller.abort());
+      (ModelClass.findAll(p, queryOptions, { signal: controller.signal }) as Promise<unknown[]>)
+        .then((results) => {
+          if (controller.signal.aborted) return;
+          setItems(reconcile(normalise(results), { key: 'id', merge: true }));
+        })
+        .catch((err) => {
+          // AbortError = a newer effect run or unmount cancelled this query.
+          // Silently drop — no UI state to update, the new run handles it.
+          if (err instanceof DOMException && err.name === 'AbortError') return;
+          throw err;
+        });
     }
   });
 
@@ -584,7 +604,21 @@ export function RenderSchema({ node, stores, registry, context = {}, children }:
             builder.subscribe(handleResults).then(handleResults);
             onCleanup(() => builder.dispose());
           } else {
-            (ModelClass.findAll(p, queryOptions) as Promise<unknown[]>).then(handleResults);
+            // Same abort discipline as the list-path createQuerySignal: the
+            // effect re-runs on perspective swap / param change, and any
+            // in-flight findAll from the previous run should be cancelled so
+            // we don't pay its serialise + transit + deserialise tax.
+            const controller = new AbortController();
+            onCleanup(() => controller.abort());
+            (ModelClass.findAll(p, queryOptions, { signal: controller.signal }) as Promise<unknown[]>)
+              .then((results) => {
+                if (controller.signal.aborted) return;
+                handleResults(results);
+              })
+              .catch((err) => {
+                if (err instanceof DOMException && err.name === 'AbortError') return;
+                throw err;
+              });
           }
         });
       }
diff --git a/packages/schema-system/frameworks/solid/tests/queryToken.test.tsx b/packages/schema-system/frameworks/solid/tests/queryToken.test.tsx
@@ -60,7 +60,7 @@ describe('$query token', () => {
     const builder = createMockBuilder();
     const MockModel = { query: vi.fn(() => builder), findAll: vi.fn() };
     const stores = {
-      spaceStore: { perspective: () => ({ uuid: 'test-perspective' }) },
+      adamStore: { currentPerspective: () => ({ uuid: 'test-perspective' }) },
       $getModel: () => MockModel,
     };
 
@@ -86,7 +86,7 @@ describe('$query token', () => {
     const builder = createMockBuilder();
     const MockModel = { query: vi.fn(() => builder), findAll: vi.fn() };
     const stores = {
-      spaceStore: { perspective: () => ({ uuid: 'test-perspective' }) },
+      adamStore: { currentPerspective: () => ({ uuid: 'test-perspective' }) },
       $getModel: () => MockModel,
     };
 
@@ -116,7 +116,7 @@ describe('$query token', () => {
     const builder = createMockBuilder();
     const MockModel = { query: vi.fn(() => builder), findAll: vi.fn() };
     const stores = {
-      spaceStore: { perspective: () => ({ uuid: 'test-perspective' }) },
+      adamStore: { currentPerspective: () => ({ uuid: 'test-perspective' }) },
       $getModel: () => MockModel,
     };
 
@@ -150,7 +150,7 @@ describe('$query token', () => {
 
     const [perspective, setPerspective] = createSignal<unknown>({ uuid: 'perspective-1' });
     const stores = {
-      spaceStore: { perspective },
+      adamStore: { currentPerspective: perspective },
       $getModel: () => MockModel,
     };
 
@@ -182,7 +182,7 @@ describe('$query token', () => {
   it('returns empty array when perspective is null', async () => {
     const MockModel = { query: vi.fn(), findAll: vi.fn() };
     const stores = {
-      spaceStore: { perspective: () => null },
+      adamStore: { currentPerspective: () => null },
       $getModel: () => MockModel,
     };
 
@@ -209,7 +209,7 @@ describe('$query token', () => {
       findAll: vi.fn(() => Promise.resolve([{ id: 1 }])),
     };
     const stores = {
-      spaceStore: { perspective: () => ({ uuid: 'p1' }) },
+      adamStore: { currentPerspective: () => ({ uuid: 'p1' }) },
       $getModel: () => MockModel,
     };
 
@@ -228,13 +228,105 @@ describe('$query token', () => {
     expect(JSON.parse(el?.textContent ?? '[]')).toEqual([{ id: 1 }]);
   });
 
+  // ---- AbortSignal threading + cleanup ----
+
+  it('passes an AbortSignal to findAll and aborts it on unmount', async () => {
+    let capturedSignal: AbortSignal | undefined;
+    const MockModel = {
+      query: vi.fn(),
+      findAll: vi.fn((_p: unknown, _q: unknown, opts: { signal?: AbortSignal } = {}) => {
+        capturedSignal = opts.signal;
+        return Promise.resolve([]);
+      }),
+    };
+    const stores = {
+      adamStore: { currentPerspective: () => ({ uuid: 'p1' }) },
+      $getModel: () => MockModel,
+    };
+
+    const node: SchemaNode = {
+      type: 'DataDisplay',
+      props: { data: { $query: { model: 'Post', subscribe: false } } },
+    };
+
+    const { unmount } = render(() => <RenderSchema node={node} stores={stores} registry={registry} />);
+    await tick();
+
+    expect(MockModel.findAll).toHaveBeenCalledOnce();
+    expect(capturedSignal).toBeInstanceOf(AbortSignal);
+    expect(capturedSignal!.aborted).toBe(false);
+
+    // Unmount should trigger onCleanup, which aborts the controller.
+    unmount();
+    expect(capturedSignal!.aborted).toBe(true);
+  });
+
+  it('aborts a stale findAll when the effect re-runs', async () => {
+    const signals: AbortSignal[] = [];
+    const MockModel = {
+      query: vi.fn(),
+      findAll: vi.fn((_p: unknown, _q: unknown, opts: { signal?: AbortSignal } = {}) => {
+        if (opts.signal) signals.push(opts.signal);
+        return new Promise<unknown[]>(() => {
+          // Never resolve — simulate slow query
+        });
+      }),
+    };
+    const [perspective, setPerspective] = createSignal<{ uuid: string } | null>({ uuid: 'p1' });
+    const stores = {
+      adamStore: { currentPerspective: perspective },
+      $getModel: () => MockModel,
+    };
+
+    const node: SchemaNode = {
+      type: 'DataDisplay',
+      props: { data: { $query: { model: 'Post', subscribe: false } } },
+    };
+
+    render(() => <RenderSchema node={node} stores={stores} registry={registry} />);
+    await tick();
+    expect(signals.length).toBe(1);
+    expect(signals[0].aborted).toBe(false);
+
+    // Trigger effect re-run by changing the reactive perspective dep.
+    setPerspective({ uuid: 'p2' });
+    await tick();
+
+    // Old run's controller aborted; new run got its own fresh signal.
+    expect(signals[0].aborted).toBe(true);
+    expect(signals.length).toBe(2);
+    expect(signals[1].aborted).toBe(false);
+  });
+
+  it('swallows AbortError from findAll without surfacing it', async () => {
+    const MockModel = {
+      query: vi.fn(),
+      findAll: vi.fn(() => Promise.reject(new DOMException('Aborted', 'AbortError'))),
+    };
+    const stores = {
+      adamStore: { currentPerspective: () => ({ uuid: 'p1' }) },
+      $getModel: () => MockModel,
+    };
+
+    const node: SchemaNode = {
+      type: 'DataDisplay',
+      props: { data: { $query: { model: 'Post', subscribe: false } } },
+    };
+
+    // If the catch arm doesn't swallow AbortError, the unhandled rejection
+    // would surface as a test failure.
+    render(() => <RenderSchema node={node} stores={stores} registry={registry} />);
+    await tick();
+    expect(MockModel.findAll).toHaveBeenCalledOnce();
+  });
+
   // ---- Query params forwarding ----
 
   it('forwards where/order/limit params to query builder', async () => {
     const builder = createMockBuilder();
     const MockModel = { query: vi.fn(() => builder), findAll: vi.fn() };
     const stores = {
-      spaceStore: { perspective: () => ({ uuid: 'p1' }) },
+      adamStore: { currentPerspective: () => ({ uuid: 'p1' }) },
       $getModel: () => MockModel,
     };
 
@@ -267,7 +359,7 @@ describe('$query token', () => {
   it('warns and returns empty array when $getModel is missing', async () => {
     const warnSpy = vi.spyOn(console, 'warn').mockImplementation(() => {});
     const stores = {
-      spaceStore: { perspective: () => ({ uuid: 'p1' }) },
+      adamStore: { currentPerspective: () => ({ uuid: 'p1' }) },
       // Note: no $getModel
     };
 
diff --git a/scripts/build-with-ad4m-link.sh b/scripts/build-with-ad4m-link.sh
