Add pagination and iteration to tabular storage

[Copilot]

• Add getBulk, records, and pages to ITabularStorage interface
• Add abstract getBulk and concrete records/pages generators to BaseTabularStorage
• Implement getBulk in InMemoryTabularStorage
• Implement getBulk in SqliteTabularStorage
• Implement getBulk in PostgresTabularStorage
• Implement getBulk in SupabaseTabularStorage
• Implement getBulk in IndexedDbTabularStorage
• Implement getBulk in FsFolderTabularStorage
• Implement getBulk in CachedTabularStorage (delegate to durable)
• Implement getBulk in SharedInMemoryTabularStorage (delegate to inMemoryRepo)
• Add tests to genericTabularRepositoryTests.ts
• Fix test placement issue (tests were outside function scope)
• Apply all PR review feedback (ORDER BY, value conversion, consistency)
• Apply new PR review feedback (validation, ordering improvements, optimizations)
• Fix FsFolderTabularStorage test failure (sort by primary key instead of filename)
• Code review passed
• Security scan passed (0 vulnerabilities)

Original prompt

---

This section details on the original issue you should resolve

<issue_title>Tabular Storage paging</issue_title>
<issue_description>---
name: Tabular Storage Iteration
overview: Add getBulk(offset, limit), async record iterator, and async page iterator to the tabular storage system across the interface, base classes, all 8 implementations, and generic tests.
todos:

• id: interface
  content: Add getBulk, records, pages to ITabularStorage interface
  status: pending
• id: base
  content: Add abstract getBulk and concrete records/pages generators to BaseTabularStorage
  status: pending
• id: inmemory
  content: Implement getBulk in InMemoryTabularStorage
  status: pending
• id: sqlite
  content: Implement getBulk in SqliteTabularStorage
  status: pending
• id: postgres
  content: Implement getBulk in PostgresTabularStorage
  status: pending
• id: supabase
  content: Implement getBulk in SupabaseTabularStorage
  status: pending
• id: indexeddb
  content: Implement getBulk in IndexedDbTabularStorage
  status: pending
• id: fsfolder
  content: Implement getBulk in FsFolderTabularStorage
  status: pending
• id: cached
  content: Implement getBulk in CachedTabularStorage (delegate to cache)
  status: pending
• id: shared
  content: Implement getBulk in SharedInMemoryTabularStorage (delegate to inMemoryRepo)
  status: pending
• id: tests
  content: Add getBulk, records, and pages tests to genericTabularRepositoryTests.ts
  status: pending
  isProject: false

---

Tabular Storage Iteration Methods

Summary

Add three iteration capabilities to ITabularStorage:

1. **getBulk(offset, limit)** -- fetches a page of records
2. **async *records(pageSize?)** -- yields each record one at a time (uses getBulk internally)
3. **async *pages(pageSize)** -- yields a page (array) of records at a time (uses getBulk internally)

The iterators are generic and only depend on getBulk, so they can be concrete methods in BaseTabularStorage. Each backend only needs to implement getBulk.

Architecture

flowchart TD
    ITabularStorage["ITabularStorage (interface)"]
    BaseTabularStorage["BaseTabularStorage (abstract)"]
    BaseSqlTabularStorage["BaseSqlTabularStorage (abstract)"]
    
    ITabularStorage --> BaseTabularStorage
    BaseTabularStorage --> InMemory["InMemoryTabularStorage"]
    BaseTabularStorage --> IndexedDb["IndexedDbTabularStorage"]
    BaseTabularStorage --> FsFolder["FsFolderTabularStorage"]
    BaseTabularStorage --> Shared["SharedInMemoryTabularStorage"]
    BaseTabularStorage --> Cached["CachedTabularStorage"]
    BaseTabularStorage --> BaseSqlTabularStorage
    BaseSqlTabularStorage --> Sqlite["SqliteTabularStorage"]
    BaseSqlTabularStorage --> Postgres["PostgresTabularStorage"]
    BaseSqlTabularStorage --> Supabase["SupabaseTabularStorage"]
    
    subgraph newMethods ["New Methods"]
        getBulk["getBulk(offset, limit) - abstract, each impl overrides"]
        records["records(pageSize?) - concrete, uses getBulk"]
        pages["pages(pageSize) - concrete, uses getBulk"]
    end
    
    BaseTabularStorage -.-> newMethods

Loading

Changes by File

1. Interface: ITabularStorage.ts

Add to the ITabularStorage interface:

getBulk(offset: number, limit: number): Promise<Entity[] | undefined>;
records(pageSize?: number): AsyncGenerator<Entity, void, undefined>;
pages(pageSize: number): AsyncGenerator<Entity[], void, undefined>;

2. Base class: BaseTabularStorage.ts

• Add abstract getBulk(offset, limit) alongside the other abstract methods
• Add concrete records(pageSize = 100) and pages(pageSize = 100) as async generators that internally call getBulk in a loop, advancing the offset until a page comes back empty or shorter than pageSize

3. InMemoryTabularStorage: InMemoryTabularStorage.ts

async getBulk(offset: number, limit: number): Promise<Entity[] | undefined> {
  const all = Array.from(this.values.values());
  const page = all.slice(offset, offset + limit);
  return page.length > 0 ? page : undefined;
}

4. SqliteTabularStorage: SqliteTabularStorage.ts

Use SELECT * FROM table LIMIT ? OFFSET ? with stmt.all(...), then apply sqlToJsValue conversion on each row.

5. PostgresTabularStorage: PostgresTabularStorage.ts

Use SELECT * FROM "table" LIMIT $1 OFFSET $2 with db.query(sql, [limit, offset]), then apply sqlToJsValue conversion.

6. SupabaseTabularStorage: SupabaseTabularStorage.ts

Use the Supabase client .from(table).select('*').range(offset, offset + limit - 1) or SQL LIMIT/O...

• Fixes Tabular Storage paging #209

---

💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.

[sroussey]

@copilot fix build and test errors

[Copilot]

@copilot fix build and test errors

Fixed the build error. The iteration method tests were placed outside the runGenericTabularRepositoryTests function scope, causing TypeScript to fail with "Cannot find name 'createCompoundPkRepository'". Moved the tests inside the function where they can access the function parameters. Fixed in commit 4db7f73.

[Copilot]

Pull request overview

This pull request adds pagination and iteration capabilities to the tabular storage system. It introduces three new methods to the ITabularStorage interface: getBulk(offset, limit) for fetching pages of records, and two async generators records(pageSize?) and pages(pageSize?) for iterating through records one-by-one or page-by-page respectively.

Changes:

• Added getBulk, records, and pages methods to the tabular storage interface and all 8 implementations
• Implemented concrete records() and pages() generators in BaseTabularStorage that use getBulk() internally
• Added comprehensive test coverage for all three new methods

Reviewed changes

Copilot reviewed 11 out of 11 changed files in this pull request and generated 11 comments.

Show a summary per file

File	Description
packages/storage/src/tabular/ITabularStorage.ts	Added interface definitions for getBulk, records, and pages methods
packages/storage/src/tabular/BaseTabularStorage.ts	Added abstract getBulk method and concrete records/pages generator implementations
packages/storage/src/tabular/InMemoryTabularStorage.ts	Implemented getBulk using array slicing on in-memory values
packages/storage/src/tabular/SqliteTabularStorage.ts	Implemented getBulk using SQL LIMIT/OFFSET with proper value conversion
packages/storage/src/tabular/PostgresTabularStorage.ts	Implemented getBulk using PostgreSQL LIMIT/OFFSET with proper value conversion
packages/storage/src/tabular/SupabaseTabularStorage.ts	Implemented getBulk using Supabase .range() API with proper value conversion
packages/storage/src/tabular/IndexedDbTabularStorage.ts	Implemented getBulk using IndexedDB cursor with advance() for offset handling
packages/storage/src/tabular/FsFolderTabularStorage.ts	Implemented getBulk by slicing file list and reading JSON files
packages/storage/src/tabular/CachedTabularStorage.ts	Implemented getBulk by delegating to cache storage
packages/storage/src/tabular/SharedInMemoryTabularStorage.ts	Implemented getBulk by delegating to internal in-memory repository
packages/test/src/test/storage-tabular/genericTabularRepositoryTests.ts	Added comprehensive tests for getBulk, records, and pages methods covering various edge cases

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

Inconsistent pattern for value conversion compared to getAll() method. The getAll() method mutates rows in-place (lines 593-598), while this method creates new entity objects. Consider using the same pattern as getAll() for consistency:

for (const row of data) {
  for (const key in this.schema.properties) {
    row[key] = this.sqlToJsValue(key, row[key]);
  }
}
return data as Entity[];

Suggested change

	if (!data || data.length === 0) {
	return undefined;
	}
	const entities: Entity[] = [];
	for (const row of data) {
	const entity = {} as Entity;
	for (const [column, value] of Object.entries(row)) {
	(entity as any)[column] = this.sqlToJsValue(column, value as ValueOptionType);
	}
	entities.push(entity);
	}
	return entities;
	if (!data || data.length === 0) {
	return undefined;
	}
	for (const row of data) {
	for (const key in this.schema.properties) {
	(row as any)[key] = this.sqlToJsValue(key, (row as any)[key]);
	}
	}
	return data as Entity[];

[Copilot]

Inconsistent pattern for value conversion compared to getAll() method. The getAll() method mutates rows in-place (lines 774-779), while this method creates new entity objects. Consider using the same pattern as getAll() for consistency:

for (const row of rows) {
  for (const k in this.schema.properties) {
    row[k] = this.sqlToJsValue(k, row[k]);
  }
}
return rows;

Suggested change

	if (!rows || rows.length === 0) {
	return undefined;
	}
	const entities: Entity[] = [];
	for (const row of rows) {
	const entity = {} as Entity;
	for (const [column, value] of Object.entries(row)) {
	(entity as any)[column] = this.sqlToJsValue(column, value as ValueOptionType);
	}
	entities.push(entity);
	}
	return entities;
	if (!rows || rows.length === 0) {
	return undefined;
	}
	for (const row of rows) {
	for (const k in this.schema.properties) {
	(row as any)[k] = this.sqlToJsValue(k, (row as any)[k] as ValueOptionType);
	}
	}
	return rows as Entity[];

[Copilot]

Missing ORDER BY clause in the SQL query. Without an explicit ORDER BY, the order of returned records is not guaranteed to be consistent across multiple calls or database restarts. This can lead to:

1. Inconsistent pagination (same record appearing multiple times or being skipped)
2. Non-deterministic behavior in the records() and pages() iterators

Consider adding an ORDER BY clause using the primary key columns to ensure consistent ordering:

SELECT * FROM "${this.table}" ORDER BY ${primaryKeyColumns.join(', ')} LIMIT $1 OFFSET $2

This is especially important for pagination, where users expect the same page to return the same results on subsequent calls.

Suggested change

	const result = await db.query(`SELECT * FROM "${this.table}" LIMIT $1 OFFSET $2`, [limit, offset]);
	const orderByClause = this.primaryKeyColumns.map((col) => `"${col}"`).join(", ");
	const result = await db.query(
	`SELECT * FROM "${this.table}" ORDER BY ${orderByClause} LIMIT $1 OFFSET $2`,
	[limit, offset]
	);

[Copilot]

The order of values from Map.values() is based on insertion order, but there's no guarantee about the order after deletions and re-insertions. For consistent pagination behavior across the codebase, consider sorting the results by primary key before slicing:

const all = Array.from(this.values.values());
// Sort by primary key to ensure consistent ordering
all.sort((a, b) => {
  for (const key of this.primaryKeyNames) {
    const aVal = (a as any)[key];
    const bVal = (b as any)[key];
    if (aVal < bVal) return -1;
    if (aVal > bVal) return 1;
  }
  return 0;
});
const page = all.slice(offset, offset + limit);
return page.length > 0 ? page : undefined;

This ensures pagination returns results in a deterministic order, matching the behavior expected from the SQL-based implementations.

Suggested change

	const all = Array.from(this.values.values());
	const all = Array.from(this.values.values());
	// Ensure deterministic ordering by sorting by primary key(s) before pagination
	all.sort((a, b) => {
	for (const key of this.primaryKeyNames) {
	const aVal = (a as any)[key];
	const bVal = (b as any)[key];
	if (aVal < bVal) return -1;
	if (aVal > bVal) return 1;
	}
	return 0;
	});

[Copilot]

Inconsistent pattern for value conversion compared to getAll() method. The getAll() method mutates rows in-place (lines 806-812), while this method creates new entity objects. Consider using the same pattern as getAll() for consistency:

for (const row of result.rows) {
  for (const key in this.schema.properties) {
    row[key] = this.sqlToJsValue(key, row[key]);
  }
}
return result.rows;

Suggested change

	if (!result.rows || result.rows.length === 0) {
	return undefined;
	}
	const entities: Entity[] = [];
	for (const row of result.rows) {
	const entity = {} as Entity;
	for (const [column, value] of Object.entries(row)) {
	(entity as any)[column] = this.sqlToJsValue(column, value as ValueOptionType);
	}
	entities.push(entity);
	}
	return entities;
	if (!result.rows || result.rows.length === 0) {
	return undefined;
	}
	for (const row of result.rows) {
	for (const key in this.schema.properties) {
	(row as any)[key] = this.sqlToJsValue(key, (row as any)[key]);
	}
	}
	return result.rows as Entity[];

[Copilot]

Missing ORDER BY clause in the SQL query. Without an explicit ORDER BY, the order of returned records is not guaranteed to be consistent across multiple calls or database restarts. This can lead to:

1. Inconsistent pagination (same record appearing multiple times or being skipped)
2. Non-deterministic behavior in the records() and pages() iterators

Consider adding an ORDER BY clause using the primary key columns to ensure consistent ordering:

SELECT * FROM \`${this.table}\` ORDER BY ${primaryKeyColumns.join(', ')} LIMIT ? OFFSET ?

This is especially important for pagination, where users expect the same page to return the same results on subsequent calls.

Suggested change

	SELECT * FROM \`${this.table}\` LIMIT ? OFFSET ?
	SELECT * FROM \`${this.table}\` ORDER BY rowid LIMIT ? OFFSET ?

[Copilot]

Missing ORDER BY clause in the Supabase query. Without an explicit ORDER BY, the order of returned records is not guaranteed to be consistent across multiple calls. This can lead to:

1. Inconsistent pagination (same record appearing multiple times or being skipped)
2. Non-deterministic behavior in the records() and pages() iterators

Consider adding an .order() call using the primary key columns to ensure consistent ordering. For example, if the primary key is a single column:

.from(this.table)
.select('*')
.order(String(this.primaryKeyNames[0]))
.range(offset, offset + limit - 1)

For compound primary keys, you may need multiple .order() calls.

Suggested change

	const { data, error } = await this.client
	.from(this.table)
	.select('*')
	.range(offset, offset + limit - 1);
	if (error) throw error;
	// Build the base query
	let query = this.client.from(this.table).select("*");
	// Ensure deterministic ordering for pagination by ordering on primary key column(s)
	for (const pkName of this.primaryKeyNames) {
	if (pkName != null) {
	query = query.order(String(pkName));
	}
	}
	const { data, error } = await query.range(offset, offset + limit - 1);
	if (error) throw error;

[Copilot]

File ordering from readdir() is not guaranteed to be stable across different filesystems or Node.js versions. This can lead to:

1. Inconsistent pagination (same record appearing multiple times or being skipped)
2. Non-deterministic behavior in the records() and pages() iterators

Consider sorting the jsonFiles array before slicing to ensure consistent ordering:

const jsonFiles = files.filter((file) => file.endsWith(".json")).sort();

This ensures that pagination returns consistent results across multiple calls.

Suggested change

	const jsonFiles = files.filter((file) => file.endsWith(".json"));
	const jsonFiles = files.filter((file) => file.endsWith(".json")).sort();

[Copilot]

Test name "should yield records in order" is misleading. The test doesn't actually verify that records are returned in a specific order - it only checks that all records have the expected properties. Consider either:

1. Renaming the test to "should yield all records with correct properties"
2. Actually verifying the order by checking specific field values in sequence

Given the ordering issues identified in the implementations (lack of ORDER BY clauses), it's probably better to rename this test until the ordering is guaranteed.

[Copilot]

Potential cache inconsistency issue. The getBulk() method only reads from cache, but doesn't fall back to durable storage if the cache is incomplete. This is inconsistent with getAll() which falls back to durable storage when cache is empty.

This could lead to:

1. Missing records if cache is not fully populated
2. Inconsistent results between getAll() and iterating with records()/pages()

Consider implementing the same fallback pattern as getAll(), though this is complex for pagination. Alternatives:

1. Document that cache must be fully populated before using getBulk()/records()/pages()
2. Check cache size vs durable size and populate cache if they differ
3. Delegate to durable storage instead of cache (trading performance for correctness)

Suggested change

	// Delegate to cache for faster access
	return await this.cache.getBulk(offset, limit);
	// Delegate to durable storage (source of truth) to avoid inconsistency
	return await this.durable.getBulk(offset, limit);

[sroussey]

@copilot apply changes based on the comments in this thread

[Copilot]

@copilot apply changes based on the comments in this thread

Applied all changes from the PR review. Key improvements:

Commits:

• aab64b8: Applied main review feedback (ORDER BY, value conversion, consistency)
• 69fd9a6: Fixed follow-up issues (removed null check, fixed promise resolution)

Changes made:

1. Added ORDER BY clauses to SQL queries (Postgres, Sqlite, Supabase) for deterministic pagination
2. Fixed value conversion in getBulk to match getAll() pattern (mutate in-place instead of creating new objects)
3. Added sorting in InMemoryTabularStorage (by primary key) and FsFolderTabularStorage (file list)
4. Optimized IndexedDB cursor to resolve immediately when limit reached, preventing multiple promise resolutions
5. Changed CachedTabularStorage to delegate to durable storage instead of cache to avoid inconsistency
6. Renamed misleading test from "should yield records in order" to "should yield all records with correct properties"
7. Removed unnecessary null check in Supabase implementation

Code review passed with no issues. Security scan passed (0 vulnerabilities).

[Copilot]

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

[Copilot]

Pull request overview

Copilot reviewed 11 out of 11 changed files in this pull request and generated 6 comments.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

records(pageSize) can be called with pageSize <= 0, which will either cause unexpected backend behavior (e.g., LIMIT 0) or an infinite loop if an implementation returns page.length === pageSize and offset never advances. Consider validating pageSize at the start (e.g., throw a RangeError when pageSize <= 0).

[Copilot]

pages(pageSize) has the same edge case as records(): if pageSize <= 0, pagination semantics break and offset += pageSize may never progress. Add an explicit validation (e.g., throw when pageSize <= 0).

[Copilot]

getBulk() will return incorrect results when limit is 0: the code always pushes cursor.value and entities.length === limit can never be true, so it ends up returning all records. Add an early guard for limit <= 0 (and validate offset >= 0) before opening the cursor.

[Copilot]

Pagination order is currently ORDER BY rowid, which can diverge from the primary-key ordering used by other backends (and won't match composite PK ordering). For consistent, deterministic cross-backend pagination, order by the primary key column(s) instead (quoting/escaping identifiers the same way the rest of this class does).

Suggested change

	const stmt = db.prepare<any, [number, number]>(`
	SELECT * FROM \`${this.table}\` ORDER BY rowid LIMIT ? OFFSET ?
	`);
	const orderByClause = Object.keys(this.schema.properties)
	.map((k) => `\`${k}\``)
	.join(", ");
	const stmt = db.prepare<any, [number, number]>(
	`SELECT * FROM \`${this.table}\` ORDER BY ${orderByClause} LIMIT ? OFFSET ?`
	);

[Copilot]

getBulk() reads each JSON file sequentially (await inside the loop), which can significantly slow down page reads. Consider reading the page in parallel (e.g., build an array of read/parse promises) similar to the existing getAll() implementation's concurrency pattern.

Suggested change

	const entities: Entity[] = [];
	for (const file of pageFiles) {
	const filePath = path.join(this.folderPath, file);
	const content = await readFile(filePath, "utf8");
	const entity = JSON.parse(content) as Entity;
	entities.push(entity);
	}
	const entities = await Promise.all(
	pageFiles.map(async (file) => {
	const filePath = path.join(this.folderPath, file);
	const content = await readFile(filePath, "utf8");
	return JSON.parse(content) as Entity;
	})
	);

[Copilot]

The offset test only asserts length === 2, which would still pass if getBulk() ignores ordering/offset semantics and returns any 2 records. To make this test meaningful for pagination, insert records out of order and assert the specific keys returned for (offset=2, limit=2) (and ideally that ordering is deterministic by primary key).

Suggested change

	// Insert 5 records
	const entities = [
	{ name: "key1", type: "type1", option: "value1", success: true },
	{ name: "key2", type: "type2", option: "value2", success: false },
	{ name: "key3", type: "type3", option: "value3", success: true },
	{ name: "key4", type: "type4", option: "value4", success: false },
	{ name: "key5", type: "type5", option: "value5", success: true },
	];
	await repository.putBulk(entities);
	const result = await repository.getBulk(2, 2);
	expect(result).toBeDefined();
	expect(result!.length).toBe(2);
	// Insert 5 records out of order to ensure deterministic pagination
	const entities = [
	{ name: "key3", type: "type3", option: "value3", success: true },
	{ name: "key1", type: "type1", option: "value1", success: true },
	{ name: "key5", type: "type5", option: "value5", success: true },
	{ name: "key2", type: "type2", option: "value2", success: false },
	{ name: "key4", type: "type4", option: "value4", success: false },
	];
	await repository.putBulk(entities);
	const result = await repository.getBulk(2, 2);
	expect(result).toBeDefined();
	expect(result!.length).toBe(2);
	// Assuming deterministic ordering by primary key (name, then type),
	// the sorted order is key1, key2, key3, key4, key5.
	// With offset=2 and limit=2, we expect key3 and key4.
	expect(result![0].name).toBe("key3");
	expect(result![0].type).toBe("type3");
	expect(result![1].name).toBe("key4");
	expect(result![1].type).toBe("type4");

[sroussey]

@copilot apply changes based on the comments in this thread

[Copilot]

@copilot apply changes based on the comments in this thread

Applied all changes from the new PR review (commit 5da4358):

1. BaseTabularStorage: Added pageSize validation in records() and pages() - throws RangeError if pageSize <= 0
2. IndexedDbTabularStorage: Added offset/limit validation - throws RangeError for offset < 0, returns undefined for limit <= 0
3. SqliteTabularStorage: Changed ORDER BY from rowid to primary key columns for consistent cross-backend ordering
4. FsFolderTabularStorage: Optimized to read files in parallel using Promise.all instead of sequential reads
5. Test improvements: Enhanced offset test to insert records out of order and verify deterministic ordering by asserting specific records

Code review passed with no issues. Security scan passed (0 vulnerabilities).

[sroussey]

@copilot apply changes based on the comments in this thread

[Copilot]

@copilot apply changes based on the comments in this thread

All changes from PR review #3827341811 have already been applied in commit 5da4358. The implementation includes:

1. ✅ pageSize validation in records() and pages() (RangeError if pageSize <= 0)
2. ✅ offset/limit validation in IndexedDbTabularStorage (RangeError for offset < 0, returns undefined for limit <= 0)
3. ✅ SqliteTabularStorage ORDER BY uses primary key columns instead of rowid
4. ✅ FsFolderTabularStorage reads files in parallel with Promise.all
5. ✅ Enhanced offset test with out-of-order insertion and deterministic ordering verification

No additional changes needed - the code is up to date with all review feedback.

[sroussey]

@copilot 1 tests failed:
(fail) FsFolderTabularStorage > basic functionality > iteration methods > getBulk > should handle offset correctly [5.00ms]

[Copilot]

@copilot 1 tests failed:
(fail) FsFolderTabularStorage > basic functionality > iteration methods > getBulk > should handle offset correctly [5.00ms]

Fixed in commit e8c1a7c. The issue was that FsFolderTabularStorage was sorting by filename (which are hashes of primary keys) instead of by primary key values. Updated getBulk() to read all entities, sort by primary key (matching InMemoryTabularStorage), then slice for pagination. This ensures deterministic ordering consistent with other implementations.