Skip to content

feat(sources): client helpers for the batched-tiles fast path [sc-556605]#302

Draft
jatorre wants to merge 2 commits into
CartoDB:mainfrom
jatorre:feat/sc-556605-batch-tiler
Draft

feat(sources): client helpers for the batched-tiles fast path [sc-556605]#302
jatorre wants to merge 2 commits into
CartoDB:mainfrom
jatorre:feat/sc-556605-batch-tiler

Conversation

@jatorre

@jatorre jatorre commented Jun 14, 2026

Copy link
Copy Markdown
Member

📊 Part of Modernizing Maps Performanceepic sc-556570 · team deck

Summary

Client half of tile-batch coalescing (server: cloud-native #25466 / sc-556575). When the source is batch-capable the tilejson now advertises a tiles_batch endpoint (cloud-native commit 4c2397b37b); a consumer coalesces a viewport's tile burst into ONE request to it instead of one warehouse-backed fetch per tile.

Adds three pure, exported helpers (mirroring the small-source slicer's shape, #301):

  • isBatchTilesTilejson(tilejson) — detect batch capability.
  • buildBatchTilesRequest(tilejson, tiles) — the sorted tiles=z/x/y,… URL (sorted ⇒ a repeated viewport is a repeated URL ⇒ as CDN-cacheable as single tiles).
  • splitBatchTilesResponse(buffer) — parse the CDTB binary container into per-tile payloads keyed by z/x/y, each ready for the format's normal tile decoder.

Tests

8 unit tests (test/sources/batch-tiler.test.ts) against a CDTB fixture built to the exact wire layout: detection, request sorting/CDN-stability, container split (incl. empty tile + bad-magic guard). Full test/sources suite green (458 / 2 skipped, no type errors).

Notes

sc-556606 (deck.gl request coalescing) consumes these; sc-556607 adopts in Builder. Part of epic #556570. Independent of #301.

…605]

Client half of tile-batch coalescing (server: cloud-native #25466 / sc-556575).
When the tilejson advertises a batch endpoint (tiles_batch), a consumer can
coalesce a viewport's tiles into one request instead of one fetch per tile.

Pure, exported helpers:
- isBatchTilesTilejson — detect batch capability.
- buildBatchTilesRequest — sorted tiles= URL (sorted ⇒ a repeated viewport is a
  repeated URL ⇒ CDN-cacheable).
- splitBatchTilesResponse — parse the CDTB container into per-tile payloads keyed
  by z/x/y, each ready for the format's normal decoder.

Tested against a CDTB fixture built to the exact wire layout. The request
coalescing itself lives in the deck.gl layer (sc-556606).
@jatorre jatorre requested a review from a team as a code owner June 14, 2026 08:04

@gemini-code-assist gemini-code-assist 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.

Code Review

This pull request introduces client-side helpers for the batched-tiles fast path, including functions to check for batch capability, build batch requests, and parse CDTB batch responses, along with corresponding unit tests. The review feedback suggests several improvements: adding explicit bounds checks in splitBatchTilesResponse to prevent out-of-bounds reads on malformed buffers, adding a defensive check in buildBatchTilesRequest to handle empty tile lists, and adding unit tests to verify error handling for truncated payloads.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

Comment on lines +84 to +95
for (let i = 0; i < tileCount; i++) {
const z = view.getUint8(offset);
offset += 1;
const x = view.getUint32(offset, true);
offset += 4;
const y = view.getUint32(offset, true);
offset += 4;
const length = view.getUint32(offset, true);
offset += 4;
tiles.set(`${z}/${x}/${y}`, data.slice(offset, offset + length));
offset += length;
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

security-high high

The binary parser in splitBatchTilesResponse does not perform bounds checks on the buffer before reading the tile headers or slicing the payload. If the input buffer is truncated or malformed, DataView will throw a generic RangeError (for out-of-bounds reads), or data.slice will silently return a truncated buffer, leading to potential data corruption or hard-to-debug errors downstream. Adding explicit bounds checks ensures robust error handling and defensive parsing of binary data.

  for (let i = 0; i < tileCount; i++) {
    if (offset + 13 > data.byteLength) {
      throw new Error('Malformed CDTB batch container: unexpected end of metadata');
    }
    const z = view.getUint8(offset);
    offset += 1;
    const x = view.getUint32(offset, true);
    offset += 4;
    const y = view.getUint32(offset, true);
    offset += 4;
    const length = view.getUint32(offset, true);
    offset += 4;
    if (offset + length > data.byteLength) {
      throw new Error('Malformed CDTB batch container: unexpected end of payload');
    }
    tiles.set(`${z}/${x}/${y}`, data.slice(offset, offset + length));
    offset += length;
  }

Comment on lines +36 to +42
export function buildBatchTilesRequest(
tilejson: TilejsonResult,
tiles: BatchTileIndex[]
): string {
if (!tilejson.tiles_batch) {
throw new Error('tilejson has no batch endpoint (tiles_batch)');
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

medium

Building a batch request with an empty list of tiles is invalid and would result in an unnecessary network request with an empty tiles= query parameter. Adding a defensive check to throw an error when tiles is empty prevents this scenario.

export function buildBatchTilesRequest(
  tilejson: TilejsonResult,
  tiles: BatchTileIndex[]
): string {
  if (!tilejson.tiles_batch) {
    throw new Error('tilejson has no batch endpoint (tiles_batch)');
  }
  if (tiles.length === 0) {
    throw new Error('Cannot build batch request with zero tiles');
  }

Comment on lines +112 to +115
test('throws on a non-CDTB buffer', () => {
const bad = new Uint8Array([0x00, 0x01, 0x02, 0x03, 0, 0, 0, 0]).buffer;
expect(() => splitBatchTilesResponse(bad)).toThrow(/CDTB/);
});

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

medium

To ensure the new bounds checks in splitBatchTilesResponse are working correctly and prevent regressions, we should add unit tests that verify the parser throws appropriate errors when encountering truncated headers or payloads.

    test('throws on a non-CDTB buffer', () => {
      const bad = new Uint8Array([0x00, 0x01, 0x02, 0x03, 0, 0, 0, 0]).buffer;
      expect(() => splitBatchTilesResponse(bad)).toThrow(/CDTB/);
    });
    test('throws on truncated header or payload', () => {
      const truncatedHeader = makeCDTB([
        {z: 0, x: 0, y: 0, payload: [1, 2, 3]},
      ]).slice(0, 15);
      expect(() => splitBatchTilesResponse(truncatedHeader)).toThrow(/unexpected end of metadata/);

      const truncatedPayload = makeCDTB([
        {z: 0, x: 0, y: 0, payload: [1, 2, 3]},
      ]).slice(0, 20);
      expect(() => splitBatchTilesResponse(truncatedPayload)).toThrow(/unexpected end of payload/);
    });

…-556605]

Addresses the review on batch-tiler.ts:

- splitBatchTilesResponse now bounds-checks before reading each 13-byte tile
  header and before slicing each payload, so a truncated or malformed CDTB
  container throws a clear 'unexpected end of metadata'/'unexpected end of
  payload' error instead of a generic DataView RangeError or a silently
  truncated payload.
- buildBatchTilesRequest rejects a zero-tile batch (would otherwise emit a
  useless request with an empty tiles= param).
- Tests: +3 (zero-tile guard, truncated header, truncated payload). Suite 11/11,
  prettier + eslint clean.
@jatorre

jatorre commented Jun 14, 2026

Copy link
Copy Markdown
Member Author

Addressed the review in 03dc8b1:

  • 🔴 no bounds checks in splitBatchTilesResponse → bounds-check before reading each 13-byte tile header (offset + 13 > byteLengthunexpected end of metadata) and before slicing each payload (offset + length > byteLengthunexpected end of payload), so a truncated/malformed CDTB container fails with a clear error instead of a generic DataView RangeError or a silently truncated payload.
  • 🟠 empty-tile batchbuildBatchTilesRequest throws on a zero-tile list instead of emitting a useless tiles= request.
  • 🟠 truncation tests → added unit tests for a truncated header and a truncated payload (plus the zero-tile guard).

Suite 11/11, prettier + eslint clean.

@jatorre jatorre marked this pull request as draft June 16, 2026 20:04
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