Skip to content

perf(map): raise tile concurrency and cancel superseded vector tile loads - #387

Closed
NearlCrews wants to merge 3 commits into
SignalK:masterfrom
NearlCrews:perf/chart-tile-throughput
Closed

perf(map): raise tile concurrency and cancel superseded vector tile loads#387
NearlCrews wants to merge 3 commits into
SignalK:masterfrom
NearlCrews:perf/chart-tile-throughput

Conversation

@NearlCrews

@NearlCrews NearlCrews commented May 28, 2026

Copy link
Copy Markdown
Contributor

What problem does this solve?

Rapid zooming on HTTP/1.1 vector chart sources leaves superseded requests consuming browser connection slots and OpenLayers tile-queue capacity. The visible zoom level can therefore wait behind tiles the user has already left.

How does it work?

The map raises maxTilesLoading from 16 to 32. Non-PMTiles VectorTile sources use an abortable loader grouped by zoom level, so a new zoom cancels requests from older levels. Every failed or canceled tile reaches the terminal ERROR state, non-success HTTP responses reject before decoding, completed controllers leave the bookkeeping map, and component teardown aborts remaining requests.

Raster and PMTiles loading are unchanged.

What changes for the user?

Vector charts recover and settle more quickly after rapid zoom changes, especially when their tile server uses HTTP/1.1. There is no new control or visual change.

How was this tested?

  • npm run test:ci -- --include src/app/modules/map/ol/lib/charts/tile-loader-abort.spec.ts: 1 file and 5 tests passed, covering success, repeated same-zoom loads, supersession, teardown, HTTP errors, fetch errors, and cleanup.
  • ESLint, Prettier, and tsc checks pass locally.
  • Signal K Plugin CI passes on Linux x64 and arm64, macOS, and Windows with Node 22 and 24, plus emulated armv7 with Node 20, on commit a916856e.
  • A real OpenLayers map used the production loader against a delayed local HTTP/1.1 MVT endpoint for 30 rapid zoom cycles. The queue started at 0, peaked at 9, returned to 0 after every cycle, and ended at 0. The server recorded 344 requests, 187 superseded-request aborts, 157 completions, 0 left in flight, and a maximum of 6 in flight. Chromium reported no console errors or page errors. Temporary instrumentation was removed.

Checklist

  • One logical change; version numbers untouched.
  • features/ untouched; the user-facing change is described above instead.
  • Skimmed DEV-LESSONS-LEARNED.md for traps relevant to this change.
  • Tests added/updated for new behavior and npm run test:ci passes.

@panaaj

panaaj commented Jun 1, 2026

Copy link
Copy Markdown
Member

According to Openlayers documentation tileLoadFunction is deprecated and an ImageTile source with a loader should be used instead.

@NearlCrews

Copy link
Copy Markdown
Contributor Author

Good catch, confirmed against the OL 10.7 docs: tileLoadFunction / setTileLoadFunction is deprecated on the raster UrlTile sources, so the XYZ, WMTS, TileWMS, and TileJSON wirings here should move to an ImageTile source with a loader. One scope note: VectorTile's tileLoadFunction is not deprecated (ImageTile is raster-only), so the vector loader stays as-is.

I'll split this: land the maxTilesLoading: 32 bump on its own since it's a one-line Map option independent of the loader API, then rework the raster cancellation onto the ImageTile loader in a follow-up. The abort/supersede bookkeeping ports over cleanly. If the ImageTile rework turns out heavier than expected I'll drop the cancellation rather than ship a deprecated path. Sound right?

@NearlCrews NearlCrews changed the title perf(map): raise tile concurrency and cancel superseded zooms perf(map): raise tile concurrency and cancel superseded vector tile loads Jun 24, 2026
@NearlCrews

Copy link
Copy Markdown
Contributor Author

Trimmed to the safe parts and rebased onto master: kept the maxTilesLoading bump and the VectorTile cancellation (VectorTile.tileLoadFunction is not deprecated), and dropped the four raster-source loaders since tileLoadFunction is deprecated there. The ImageTile-based raster rework can be a follow-up if you want it.

@NearlCrews
NearlCrews force-pushed the perf/chart-tile-throughput branch from 9228386 to df0f5e1 Compare June 24, 2026 01:58
@joelkoz

joelkoz commented Jun 26, 2026

Copy link
Copy Markdown
Collaborator

Thanks for trimming this down — splitting off the deprecated raster tileLoadFunction wirings and keeping just the maxTilesLoading bump plus the VectorTile cancellation (which, you're right, isn't deprecated) was exactly the right call, and the rebase onto master is clean. The root-cause writeup on the HTTP/1.1 connection cap is genuinely helpful context too.

My Claude asked your Claude for this:

# Verification — PR #387 (vector tile abort + maxTilesLoading)

Context: this PR replaces OpenLayers' built-in VectorTile load path with a
custom `tileLoadFunction` (`createAbortableVectorTileLoader` in
`src/app/modules/map/ol/lib/charts/tile-loader-abort.ts`) that cancels
in-flight tile loads from superseded zoom levels, and bumps
`maxTilesLoading` from 16 to 32 in `map.component.ts`.

The current "Tested" evidence is `tsc --noEmit` + `build:web` — compile only.
There are two gaps to close: (1) a unit test proving the abort bookkeeping,
and (2) runtime proof the vector charts still render and requests cancel.

---

## Part 1 — Add a vitest spec for the abort bookkeeping

FSK runs vitest with co-located `*.spec.ts`. Add
`src/app/modules/map/ol/lib/charts/tile-loader-abort.spec.ts` proving the
per-zoom supersede logic. The pure helpers (`bucketForZoom`,
`releaseController`) are the cleanest seam — export them from the module
(keep `createAbortableVectorTileLoader` as the public entry) so they can be
tested directly without mocking the whole OL VectorTile.

Cases to cover:
- **Same-zoom reuse:** two calls at zoom Z return the *same* controller set;
  no controllers are aborted.
- **Supersede aborts others:** with controllers held under zoom Z1, a call
  for zoom Z2 aborts every Z1 controller (assert `.signal.aborted === true`)
  and removes the Z1 bucket from the map.
- **Release cleanup:** removing the last controller for a zoom deletes that
  zoom's bucket entirely; removing one of several leaves the rest intact.
- **Release is zoom-safe:** releasing a controller under a zoom that's
  already been superseded/deleted is a no-op, not a throw.

Run `ng test` (or the vitest invocation this repo uses) and confirm green.

---

## Part 2 — Runtime verification

### Setup
1. Check out the PR branch (`perf/chart-tile-throughput`) and `ng serve`.
2. Point FSK at a Signal K server with a **vector (MVT) chart source**
   served over **HTTP/1.1** (the connection-cap condition this PR targets —
   confirm in DevTools the tile responses are h1, not h2).
3. Open FSK in Chrome → DevTools → Network, filter to the tile host,
   enable "Disable cache".

### Checks (capture an artifact for each)

A. **Vector charts still render correctly.**
   Load the vector chart; pan and zoom across a few levels. Confirm features
   draw with no missing/blank tiles and no console errors.
   → Artifact: screenshot of the rendered vector chart + clean console.

B. **Superseded requests actually cancel.**
   Zoom out across 2–3 levels rapidly (faster than tiles finish). In the
   Network panel, confirm tile requests from the abandoned zoom level(s)
   show status **(canceled)**.
   → Artifact: Network-waterfall screenshot showing (canceled) rows for the
     superseded zoom, plus the in-flight count for the visible level.

C. **No regression in the steady state.**
   Zoom to a level and let it fully settle. Confirm every tile reaches
   LOADED and nothing is stuck in (canceled) or error.
   → Artifact: waterfall of a clean settle (all 200/LOADED).

D. **maxTilesLoading effect (optional).**
   On the same HTTP/1.1 source, compare time-to-visually-settle after a
   zoom-out burst on this branch vs. master, same hardware.
   → Artifact: before/after settle time (or a short screen capture).

### Edge cases to spot-check
- Cross-origin source: does plain `fetch(src)` still load (CORS /
  credentials) the same as OL's default loader did? No CORS errors for a
  remote chart host.
- Rapid pinch-zoom through intermediate levels: the final level renders and
  you don't end up with a level whose tiles were all aborted and never
  re-requested.

---

## Output
- Confirm the new spec passes (`ng test`).
- Post the runtime artifacts on PR #387, one line each on what they show.
- If any check fails, note which and the console/network detail.

@joelkoz joelkoz self-assigned this Jun 26, 2026
@coderabbitai

coderabbitai Bot commented Jun 27, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: c608c3dc-6610-430c-94ed-047359df3c36

📥 Commits

Reviewing files that changed from the base of the PR and between 38ba137 and f888bf8.

📒 Files selected for processing (1)
  • src/app/modules/map/ol/lib/charts/tile-loader-abort.spec.ts
🚧 Files skipped from review as they are similar to previous changes (1)
  • src/app/modules/map/ol/lib/charts/tile-loader-abort.spec.ts

📝 Walkthrough

Walkthrough

Adds an abortable vector-tile loader with per-zoom controller tracking. Superseded requests are aborted, tile states update after fetch results, and chart layers cancel pending loads during destruction. The map allows up to 32 concurrent tile loads.

Changes

Abortable Vector Tile Loading

Layer / File(s) Summary
PendingByZoom state and loader factory
src/app/modules/map/ol/lib/charts/tile-loader-abort.ts
Tracks AbortController instances by zoom. It aborts superseded requests, fetches and decodes tile data, updates tile states, and releases controllers.
Layer and Map wiring
src/app/modules/map/ol/lib/charts/layer-vector-chart.component.ts, src/app/modules/map/ol/lib/map.component.ts
Wires the loader into non-PMTiles vector layers, aborts pending loads during destruction, removes an unnecessary cast, and sets maxTilesLoading to 32.
Abort behavior validation
src/app/modules/map/ol/lib/charts/tile-loader-abort.spec.ts
Tests successful decoding, same-zoom requests, superseded-zoom abortion, global cancellation, HTTP errors, fetch failures, and cleanup.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Sequence Diagram(s)

sequenceDiagram
  participant MapComponent
  participant VectorChartLayerComponent
  participant VectorTileSource
  participant TileLoader
  participant Fetch

  MapComponent->>MapComponent: Create OpenLayers Map with maxTilesLoading=32
  VectorChartLayerComponent->>TileLoader: Create abortable tile loader
  VectorChartLayerComponent->>VectorTileSource: Set tileLoadFunction
  VectorTileSource->>TileLoader: Load vector tile
  TileLoader->>Fetch: Fetch tile data with AbortSignal
  Fetch-->>TileLoader: Return array-buffer data
  TileLoader-->>VectorTileSource: Set features and tile state
  VectorChartLayerComponent->>TileLoader: Abort pending loads on destroy
Loading
🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 33.33% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
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.
Title check ✅ Passed The title clearly summarizes the two main changes: higher tile concurrency and cancellation of superseded vector tile loads.
Description check ✅ Passed The description covers the problem, implementation, user impact, testing, and checklist, with details that match the pull request changes.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

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

@NearlCrews

Copy link
Copy Markdown
Contributor Author

Both parts are in.

Part 1: unit spec. Added tile-loader-abort.spec.ts (vitest). bucketForZoom and releaseController were already the right seam, so I exported them plus the PendingByZoom type, with no runtime change. Nine cases:

  • same zoom returns the same bucket, and a same-zoom repeat aborts nothing
  • a new zoom aborts every controller in the superseded buckets (signal.aborted === true) and deletes those buckets, including several stale zooms in one call
  • release deletes a bucket on its last controller, keeps it when others remain, and is a no-op for an already-removed zoom or a stranger controller

Green at 9 passed. (app.component.spec.ts fails independently on the src/app/modules path alias in the vitest config, unrelated to this branch, confirmed by stashing.)

Part 2: runtime. I drove the actual createAbortableVectorTileLoader, wired exactly as layer-vector-chart.component wires it (as the VectorTileSource tileLoadFunction), in headless Chromium against real OpenFreeMap MVT tiles proxied through a plain Node server, so the browser fetches them over HTTP/1.1 with 350 ms of added latency. That is the connection-cap condition this PR targets. Then a rapid zoom out from z12 to z6, faster than the latency, to force in-flight supersession.

Results (request log, 63 tile requests):

  • Renders: the vector chart draws correctly at z12 (central London) with zero console errors.
  • Superseded loads cancel: 15 in-flight tile loads at the superseded zooms (6 at z9, 9 at z10) ended in net::ERR_ABORTED, with 0 non-abort failures.
  • Clean settle: at the final z6 all 12 visible tiles reached LOADED with 0 errors, so no level is left all-aborted-and-never-re-requested.

This is a synthetic harness exercising the real loader code path, not FSK driven by hand against a Signal K vector source. If you would rather see it against a live SK chart source, or want the render and waterfall screenshots and the harness script, say the word and I will post them.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🧹 Nitpick comments (1)
src/app/modules/map/ol/lib/charts/tile-loader-abort.spec.ts (1)

12-120: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Add specs for createAbortableVectorTileLoader().

These tests only cover the helper map/set bookkeeping. The new behavior that actually ships here—fetch cancellation, AbortError handling, tile state transitions, and cleanup after settle—is still untested, so the main regression surface from this PR would slip through.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/app/modules/map/ol/lib/charts/tile-loader-abort.spec.ts` around lines 12
- 120, The current spec only covers bucket bookkeeping in bucketForZoom and
releaseController, but not the actual loader behavior introduced by
createAbortableVectorTileLoader(). Add tests around
createAbortableVectorTileLoader that verify an in-flight fetch is canceled via
AbortController, AbortError is handled without failing the tile, tile state
transitions happen correctly on success/failure, and cleanup occurs after the
request settles. Use the existing helper patterns in the spec to locate and
exercise the loader entry points directly.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@src/app/modules/map/ol/lib/charts/tile-loader-abort.ts`:
- Around line 52-58: Expose a teardown hook from createAbortableVectorTileLoader
so callers can abort all pending controllers held in the pending map. Update the
loader factory and the createAbortableVectorTileLoader return type to include a
cleanup/teardown function, and use it from
VectorChartLayerComponent.ngOnDestroy() to cancel any still-running tile fetches
before the layer is removed.
- Around line 80-86: The abort handling in tile-loader-abort should put
superseded tiles into a terminal state instead of leaving them stuck in LOADING.
Update the abort path around the tile load promise and the tile state logic in
tile-loader-abort so AbortError is treated as a terminal outcome and the tile is
set to ERROR via tile.setState, while keeping EMPTY reserved for intentionally
blank/disposed tiles. Use the existing tile.setState, TileState.ERROR, and abort
catch/finally flow to locate the fix.

---

Nitpick comments:
In `@src/app/modules/map/ol/lib/charts/tile-loader-abort.spec.ts`:
- Around line 12-120: The current spec only covers bucket bookkeeping in
bucketForZoom and releaseController, but not the actual loader behavior
introduced by createAbortableVectorTileLoader(). Add tests around
createAbortableVectorTileLoader that verify an in-flight fetch is canceled via
AbortController, AbortError is handled without failing the tile, tile state
transitions happen correctly on success/failure, and cleanup occurs after the
request settles. Use the existing helper patterns in the spec to locate and
exercise the loader entry points directly.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: b7783283-a22a-400f-9956-6672af3a1421

📥 Commits

Reviewing files that changed from the base of the PR and between 8f8875d and 4a814f8.

📒 Files selected for processing (4)
  • src/app/modules/map/ol/lib/charts/layer-vector-chart.component.ts
  • src/app/modules/map/ol/lib/charts/tile-loader-abort.spec.ts
  • src/app/modules/map/ol/lib/charts/tile-loader-abort.ts
  • src/app/modules/map/ol/lib/map.component.ts

Comment thread src/app/modules/map/ol/lib/charts/tile-loader-abort.ts Outdated
Comment thread src/app/modules/map/ol/lib/charts/tile-loader-abort.ts Outdated
@joelkoz

joelkoz commented Jun 29, 2026

Copy link
Copy Markdown
Collaborator

This is exactly the evidence I was after — thank you for the thorough turnaround. The vitest cases nail the supersede bookkeeping, and the headless run over a real HTTP/1.1 + latency path is a clean way to prove the two things I couldn't take on faith from a green build: vector charts still render, and superseded loads actually cancel (the 15 ERR_ABORTED at z9/z10 with a clean z6 settle is precisely the behavior the PR claims). No need for the live-SK rerun on my account — the synthetic harness exercises the real loader code path, which is what matters. Nice catch confirming the app.component.spec.ts failure is a pre-existing path-alias issue, not your branch.

CodeRabbit has since flagged two things I'd treat as substantive rather than style, and I'd like both addressed before merge:

  1. Aborted tiles aren't moved to a terminal state (tile-loader-abort.ts ~80-86). The .catch only sets ERROR for non-AbortError, so a superseded tile is left sitting in LOADING. Worth confirming OL re-requests those on the next render rather than stranding them — this is the one I'd most want resolved.
  2. No teardown on unmount (~52-58). Pending controllers aren't aborted when the layer is destroyed; exposing a cleanup that VectorChartLayerComponent.ngOnDestroy() calls closes that.

Its third note (test createAbortableVectorTileLoader() itself, not just the helpers) is a nitpick — your Part-2 harness already covers that behaviorally, so promoting a slice of it into a committed test would more than satisfy it if you feel like it.

Since you're on Claude Code: CodeRabbit drops a "🤖 Prompt for AI Agents" block under each finding, plus a combined "Prompt for all review comments with AI agents" at the end of its review — those are written to paste straight into your agent, so you don't have to translate the comments by hand. Get those two items sorted and I think this is ready to land.

@joelkoz

joelkoz commented Jul 28, 2026

Copy link
Copy Markdown
Collaborator

@coderabbitai reviews remaining

@coderabbitai

coderabbitai Bot commented Jul 28, 2026

Copy link
Copy Markdown

Your plan includes PR reviews subject to rate limits. More reviews will be available in 17 minutes.

@joelkoz

joelkoz commented Jul 28, 2026

Copy link
Copy Markdown
Collaborator

@NearlCrews For your AI Agent:

# PR #387 — finish the two open CodeRabbit items

You are working on branch `perf/chart-tile-throughput` in
`SignalK/freeboard-sk`, currently at head `833a4b6`.

## State of play (verify before you start)

The 2026-07-27 push was a **rebase onto master with no source change**.
`src/app/modules/map/ol/lib/charts/tile-loader-abort.ts` is byte-identical to
what CodeRabbit reviewed at `4a814f8`, and `VectorChartLayerComponent.ngOnDestroy()`
is unchanged. Both actionable CodeRabbit findings from the 2026-06-27 review are
still open. Confirm this yourself against the current working tree before
concluding anything is already done — if the fixes exist on a different local
branch, the wrong branch was pushed.

Do **not** post `@coderabbitai review` to re-trigger a review. The rebase carried
no source change, so a re-review would re-post the identical findings and consume
a rate-limited slot for nothing. Push the fixes first; the push itself triggers
the review.

## Task 1 (merge gate) — give aborted tiles a terminal state

In `tile-loader-abort.ts`, the catch block is currently:

```ts
.catch((err) => {
  if (err?.name !== 'AbortError') {
    tile.setState(TileState.ERROR);
  }
})
```

On abort the tile is left in `TileState.LOADING` permanently. Move it to a
terminal state (`TileState.ERROR`; reserve `EMPTY` for intentionally blank or
disposed tiles).

**Why this is the gate and not a tidiness nit.** OpenLayers' tile queue counts
tiles in `LOADING` against the `maxTilesLoading` budget. If every supersede
strands a tile in `LOADING`, the budget monotonically erodes over a session of
repeated zooms — which would undermine the exact concurrency headroom this PR
raises from 16 to 32. The existing runtime harness cannot detect this: it runs a
single zoom sequence and checks the final settle, so a slow leak across many
cycles is invisible to it.

**Verify empirically, don't just reason about it.** Instrument the map's
tiles-loading count (OL exposes the tile queue's loading count; find the accessor
in the installed `ol` version rather than assuming an API shape) and run several
dozen rapid zoom cycles. Acceptance: the count returns to zero when the map
settles, and does not ratchet upward across cycles. Report the before/after
numbers — if the count was already returning to zero on the current code, say so
explicitly, because that would mean OL reclaims these tiles some other way and
the severity here is lower than stated.

## Task 2 — teardown on layer destroy

`createAbortableVectorTileLoader()` currently returns only the loader function,
so nothing aborts the controllers still held in the `pending` map when a layer
goes away. Expose a cleanup/teardown alongside the loader and call it from
`VectorChartLayerComponent.ngOnDestroy()` (currently lines ~51-57, which only do
`removeLayer` + `render`) so in-flight fetches are cancelled before the layer is
removed. This compounds with Task 1: a destroyed layer's tiles are stranded the
same way.

## Task 3 (optional, closes the CodeRabbit nitpick)

`tile-loader-abort.spec.ts` covers `bucketForZoom` and `releaseController` well —
that part is done and is a genuine regression guard. It does not cover
`createAbortableVectorTileLoader()` itself, which is where both fixes above land.
Add cases asserting: an in-flight fetch is cancelled via its `AbortController`,
an `AbortError` drives the tile to a terminal state, success drives it to
`LOADED`, and the controller is released from `pending` after settle. A slice of
the existing runtime harness promoted into a committed test would cover this.

## Definition of done
- Both fixes pushed as commits on this branch (incremental commits are fine).
- `ng test` green.
- The zoom-cycle measurement from Task 1 reported in a PR comment, with numbers.
- If you conclude any finding is not valid against the current code, skip it and
  say why rather than changing code to satisfy it.

The two items above are what's between this and merge — everything else is in good shape (CI fully green including the slow armv7 Cerbo job, and the abort-bookkeeping spec from the last round does its job). One heads-up: the CodeRabbit check on the current head reads green, but that's a rate-limited non-review, not an approval — worth knowing so it doesn't read as all-clear.

@coderabbitai

coderabbitai Bot commented Jul 28, 2026

Copy link
Copy Markdown

Tip

For best results, initiate chat on the files or code changes.

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo '== branch and relevant commits =='
git status --short
git branch --show-current
git rev-parse --short HEAD
git diff --no-ext-diff 4a814f8..HEAD -- \
  src/app/modules/map/ol/lib/charts/tile-loader-abort.ts \
  src/app/modules/map/ol/lib/charts/layer-vector-chart.component.ts || true

echo '== source outlines =='
ast-grep outline src/app/modules/map/ol/lib/charts/tile-loader-abort.ts --items all
ast-grep outline src/app/modules/map/ol/lib/charts/layer-vector-chart.component.ts --items all
ast-grep outline src/app/modules/map/ol/lib/map.component.ts --items all

echo '== targeted source =='
cat -n src/app/modules/map/ol/lib/charts/tile-loader-abort.ts
cat -n src/app/modules/map/ol/lib/charts/layer-vector-chart.component.ts
cat -n src/app/modules/map/ol/lib/charts/tile-loader-abort.spec.ts
sed -n '1,220p' package.json

echo '== test config and OpenLayers dependency locations =='
fd -HI '^(vite|vitest|angular)\.config\..*$|^package\.json$|^TileQueue\.(d\.ts|js)$' . \
  -E .git -E dist -E build -E node_modules/.cache || true
rg -n -C 3 --glob 'package.json' --glob '*.{ts,js,mts,cts}' \
  '(vitest|ng test|TileQueue|tilesLoading|tilesLoading_|getTilesLoading)' \
  . -g '!node_modules/.cache/**' -g '!dist/**' -g '!build/**' || true

echo '== installed OpenLayers tile queue declarations/source, if present =='
if [ -d node_modules/ol ]; then
  rg -n -C 4 '(getTilesLoading|tilesLoading|tilesLoading_)' node_modules/ol \
    -g 'TileQueue.*' -g 'Map.*' || true
fi

Length of output: 46806


🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo '== tracked performance/runtime harness and test configuration =='
fd -HI . dev-tools/perf-harness .github 2>/dev/null || true
ast-grep outline vitest-base.config.ts --items all 2>/dev/null || true
cat -n vitest-base.config.ts 2>/dev/null || true
find dev-tools/perf-harness -maxdepth 3 -type f -print 2>/dev/null | sort | while read -r file; do
  echo "----- $file"
  sed -n '1,260p' "$file"
done

echo '== OpenLayers queue API references in tracked code =='
rg -n -C 5 --glob '*.{ts,js,mts,cts}' \
  '(getTileQueue\s*\(|getTilesLoading\s*\(|tilesLoading_)' \
  src dev-tools .github || true

echo '== GitHub PR and commits =='
gh pr view 387 --repo SignalK/freeboard-sk \
  --json number,headRefName,headRefOid,commits,files \
  --jq '{number, headRefName, headRefOid, commits: [.commits[] | {oid: .oid, messageHeadline: .messageHeadline}], files: [.files[] | .path]}'
gh api repos/SignalK/freeboard-sk/commits/4a814f8 \
  --jq '{sha, files: [.files[] | select(.filename == "src/app/modules/map/ol/lib/charts/tile-loader-abort.ts" or .filename == "src/app/modules/map/ol/lib/charts/layer-vector-chart.component.ts") | {filename, sha, status}]}' \
  || true

Length of output: 59304


🤖 web_search executed:

OpenLayers v10.7.0 Map getTileQueue TileQueue getTilesLoading API documentation

Length of output: 7381


🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo '== current loader blob vs reviewed loader blob =='
printf 'current git blob: '
git hash-object src/app/modules/map/ol/lib/charts/tile-loader-abort.ts
echo 'reviewed GitHub blob: a4ed22a4fa54deb67fb952d5574a7ba0c3434ae0'
gh api repos/SignalK/freeboard-sk/git/blobs/a4ed22a4fa54deb67fb952d5574a7ba0c3434ae0 \
  --jq '.content' | tr -d '\n' | base64 -d | sha1sum

echo '== OpenLayers v10.7.0 Map and TileQueue APIs =='
for path in src/ol/Map.js src/ol/TileQueue.js; do
  echo "----- $path"
  gh api "repos/openlayers/openlayers/contents/$path?ref=v10.7.0" \
    --jq '.content' | tr -d '\n' | base64 -d \
    | rg -n -C 5 '(getTileQueue|getTilesLoading|tileQueue_|loadMoreTiles|class TileQueue)'
done

Length of output: 1257


🏁 Script executed:

#!/bin/bash
set -euo pipefail

for path in src/ol/Map.js src/ol/Map.d.ts src/ol/TileQueue.js src/ol/TileQueue.d.ts; do
  echo "----- $path"
  gh api "repos/openlayers/openlayers/contents/$path?ref=v10.7.0" \
    --jq '.content' 2>/dev/null \
    | tr -d '\n' \
    | base64 -d 2>/dev/null \
    | rg -n -C 5 '(getTileQueue|getTilesLoading|tileQueue_|loadMoreTiles|class TileQueue)' \
    || true
done

Length of output: 4069


@joelkoz: Confirmed: both merge-gating findings remain open on 833a4b6. tile-loader-abort.ts is byte-identical to the reviewed blob, aborted requests are left in LOADING, and VectorChartLayerComponent.ngOnDestroy() has no cancellation path.

OpenLayers 10.7 has TileQueue#getTilesLoading(), but Map owns it as private tileQueue_; there is no supported Map#getTileQueue() accessor. The runtime measurement should therefore use temporary, non-committed postrender instrumentation that records event.frameState.tileQueue.getTilesLoading()—not a production dependency on tileQueue_.

  • Convert the loader factory from a bare callback into a small lifecycle-owned object containing tileLoadFunction and dispose().
  • Mark every failed fetch—including AbortError—as TileState.ERROR, so all aborted tiles leave LOADING.
  • Retain the loader object in VectorChartLayerComponent and invoke its disposal before removing the vector layer.
  • Add loader-level Vitest cases for success, supersession aborts, terminal abort state, release-after-settle, and explicit disposal.
  • Validate with ng test, then run repeated rapid zoom cycles using temporary postrender queue instrumentation and report the initial, peak, and settled counts.

Failed to handle agent chat message. Please try again.

@coderabbitai

coderabbitai Bot commented Jul 28, 2026

Copy link
Copy Markdown

Tip

For best results, initiate chat on the files or code changes.

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo '== relevant source outlines =='
ast-grep outline src/app/modules/map/ol/lib/charts/tile-loader-abort.ts --items all
ast-grep outline src/app/modules/map/ol/lib/charts/layer-vector-chart.component.ts --items all
ast-grep outline src/app/modules/map/ol/lib/charts/tile-loader-abort.spec.ts --items all

echo '== current relevant source =='
cat -n src/app/modules/map/ol/lib/charts/tile-loader-abort.ts
cat -n src/app/modules/map/ol/lib/charts/layer-vector-chart.component.ts
cat -n src/app/modules/map/ol/lib/charts/tile-loader-abort.spec.ts

echo '== loader factory definitions and call sites =='
rg -n -C 4 --glob '*.{ts,tsx}' \
  '\bcreateAbortableVectorTileLoader\s*\(|\btileLoadFunction\s*:' \
  src/app/modules/map/ol/lib

Length of output: 18225


@joelkoz: Confirmed against the current branch: both merge-gating findings are still present.

  • Aborts strand tiles in LOADING: tile-loader-abort.ts Lines 80-83 only sets ERROR for non-AbortError failures.
  • No component teardown: VectorChartLayerComponent.ngOnDestroy() at Lines 51-57 removes the layer without cancelling its loader’s pending requests.
  • Scope is contained: createAbortableVectorTileLoader() has exactly one consumer, at layer-vector-chart.component.ts Line 90. PMTiles uses separate loader wiring and can remain unchanged.
  • Test gap confirmed: the existing nine specs cover bookkeeping helpers only; none exercise the loader factory’s fetch, abort, terminal-state, release, or dispose paths.

The appropriate implementation is therefore to return a { tileLoadFunction, dispose } lifecycle object, always transition rejected loads—including AbortError—to TileState.ERROR, retain/dispose that object before removeLayer, and add factory-level deferred-fetch tests. The local HTTP/1.1 repeated-zoom measurement remains required to prove the tile queue settles back to zero without ratcheting across cycles.

@joelkoz joelkoz added the awaiting response Requires further response from submitter. label Aug 9, 2026
@NearlCrews
NearlCrews force-pushed the perf/chart-tile-throughput branch from 833a4b6 to 3243f7f Compare August 12, 2026 05:23
@coderabbitai

coderabbitai Bot commented Aug 12, 2026

Copy link
Copy Markdown

Note

GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 3

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@src/app/modules/map/ol/lib/charts/tile-loader-abort.spec.ts`:
- Around line 7-13: Update the tests in the tile-loader-abort spec to import and
exercise only the public createAbortableVectorTileLoader factory, removing
imports and direct suites for PendingByZoom, bucketForZoom, and
releaseController. Cover repeated and superseded zoom loads, verify fetch abort
signals and tile states, and validate teardown behavior through the factory’s
public contract.

In `@src/app/modules/map/ol/lib/charts/tile-loader-abort.ts`:
- Around line 82-83: Update the fetch chain in the tile-loading function around
fetch and r.arrayBuffer() to check Response.ok before reading the body,
rejecting non-success responses so they reach the existing catch path and set
TileState.ERROR. Add a factory-level test that verifies an HTTP 404 or 500
response produces the error tile state.
- Around line 90-91: Remove the explicit any cast in the VectorTileLoadFunction
callback and call setFeatures directly on the tile parameter, preserving the
existing features argument and behavior.
🪄 Autofix

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 7c56e183-482d-4e67-8688-b9abadeb1f12

📥 Commits

Reviewing files that changed from the base of the PR and between 281a61d and 3243f7f.

📒 Files selected for processing (4)
  • src/app/modules/map/ol/lib/charts/layer-vector-chart.component.ts
  • src/app/modules/map/ol/lib/charts/tile-loader-abort.spec.ts
  • src/app/modules/map/ol/lib/charts/tile-loader-abort.ts
  • src/app/modules/map/ol/lib/map.component.ts
🚧 Files skipped from review as they are similar to previous changes (1)
  • src/app/modules/map/ol/lib/map.component.ts

Comment thread src/app/modules/map/ol/lib/charts/tile-loader-abort.spec.ts Outdated
Comment thread src/app/modules/map/ol/lib/charts/tile-loader-abort.ts Outdated
Comment thread src/app/modules/map/ol/lib/charts/tile-loader-abort.ts Outdated
@NearlCrews
NearlCrews force-pushed the perf/chart-tile-throughput branch from 3243f7f to 8cd4e74 Compare August 12, 2026 05:55
…oads

Raise OL maxTilesLoading from the default 16 to 32 so zoom-out tile
bursts keep the browser's HTTP/1.1 connection pool fed.

Add an abortable VectorTile tileLoadFunction that cancels in-flight
loads from superseded zoom levels, freeing the connection pool for the
now-visible level.

Raster-source cancellation is intentionally left out: OL deprecates
tileLoadFunction on raster (UrlTile) sources in favor of an ImageTile
loader, which is a larger rework for a follow-up.
@NearlCrews
NearlCrews force-pushed the perf/chart-tile-throughput branch from 8cd4e74 to 38ba137 Compare August 12, 2026 06:34

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@src/app/modules/map/ol/lib/charts/tile-loader-abort.spec.ts`:
- Around line 63-89: Update the fetchMock implementation in the successful
request test to preserve the Promise<Response> mock behavior without declaring
unused _src or _init parameters. Keep the existing successful response and fetch
call typing intact, and leave the surrounding tile-loading assertions unchanged.
🪄 Autofix

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: ee5a792a-cc1e-4212-83e0-1feb98339510

📥 Commits

Reviewing files that changed from the base of the PR and between 3243f7f and 38ba137.

📒 Files selected for processing (2)
  • src/app/modules/map/ol/lib/charts/tile-loader-abort.spec.ts
  • src/app/modules/map/ol/lib/charts/tile-loader-abort.ts

Comment thread src/app/modules/map/ol/lib/charts/tile-loader-abort.spec.ts
@NearlCrews

Copy link
Copy Markdown
Contributor Author

Repeated rapid-zoom queue measurement is complete. I ran the production createAbortableVectorTileLoader in a real OpenLayers map against a delayed local HTTP/1.1 MVT endpoint for 30 cycles.

  • queue before cycling: 0
  • peak queue loading count: 9
  • loading count after each settle: 0 for all 30 cycles
  • final queue loading count: 0
  • endpoint totals: 344 requests, 187 aborted as superseded, 157 completed, 0 in flight, maximum 6 in flight
  • Chromium: 0 console errors and 0 page errors

The settled counts never ratcheted upward. I removed the temporary postrender queue sampling and HTTP harness after the run.

@NearlCrews

Copy link
Copy Markdown
Contributor Author

Closing because this branch targets code that has since changed, so the proposal is no longer current.

@NearlCrews NearlCrews closed this Aug 17, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

awaiting response Requires further response from submitter.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants