From e8ee4ee6874ce4180c817278fc68b24673240ad6 Mon Sep 17 00:00:00 2001 From: Mostafa Sadeghi <205455727+mostafasadeghidev@users.noreply.github.com> Date: Thu, 6 Aug 2026 10:05:49 +0200 Subject: [PATCH 1/5] feat(loops): filter data-row loops by a cell value MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A loop could pick a table and an order but not WHICH rows, so a section that should list the three featured articles listed the three newest ones. Migrated Webflow sites hit this immediately: their lists are curated by a boolean field the loop had no way to read. - New `@core/loops/cellFilter`: parse one condition out of the loop's filter bag and render it as SQL. Six operators (is / is not / checked / unchecked / has any value / empty), a closed set — never interpolated. - Both query paths apply it (post-type version join and data-kind direct read) and so do their COUNT queries, or pagination advertises rows the page query drops. - The canvas preview endpoint takes the same condition, so the editor shows what the published page will emit. - Properties panel renders a field picker from the selected table, and hides the value box for the operators that ignore it. Dialect notes, both learned the hard way and now pinned by tests: SQLite binds `?` by position in the TEXT, so the condition's parameters sit between tableId and limit/offset; and its json_extract returns INTEGER 1/0 for booleans, which never equals '1' across storage classes — hence the cast. The JSON read appears exactly once per fragment so the field name binds once, with coalesce folding in rows that lack the field. The field NAME binds as a parameter like the value, so no part of a filter reaches the statement text. Co-Authored-By: Claude Fable 5 --- server/handlers/cms/data/tables.ts | 10 ++ src/__tests__/loops/cellFilter.test.ts | 165 ++++++++++++++++++ .../loops/dataRowsCellFilter.test.ts | 165 ++++++++++++++++++ .../pages/site/canvas/useLoopPreviewItems.ts | 9 +- .../PropertiesPanel/LoopPropertiesView.tsx | 25 ++- src/core/loops/cellFilter.ts | 148 ++++++++++++++++ src/core/loops/sources/dataRows.ts | 101 ++++++++--- src/core/persistence/cmsData.ts | 7 + 8 files changed, 608 insertions(+), 22 deletions(-) create mode 100644 src/__tests__/loops/cellFilter.test.ts create mode 100644 src/__tests__/loops/dataRowsCellFilter.test.ts create mode 100644 src/core/loops/cellFilter.ts diff --git a/server/handlers/cms/data/tables.ts b/server/handlers/cms/data/tables.ts index 521a87339..083968058 100644 --- a/server/handlers/cms/data/tables.ts +++ b/server/handlers/cms/data/tables.ts @@ -40,6 +40,7 @@ import { normalizeDataTableFields } from '@core/data/fields' import { slugForTable } from '@core/data/cells' import { slugFromTitle } from '@core/utils/slug' import { fetchPublishedDataRowItems } from '@core/loops/sources/dataRows' +import { parseCellFilter } from '@core/loops/cellFilter' import { badRequest, jsonResponse, methodNotAllowed, readValidatedBody } from '../../../http' import { CMS_API_PREFIX, requestAuditContext } from '../shared' import { @@ -382,12 +383,21 @@ async function handleTableLoopPreview( const rawOffset = Number.parseInt(url.searchParams.get('offset') ?? '0', 10) const offset = Math.max(Number.isFinite(rawOffset) ? rawOffset : 0, 0) + // The canvas preview must apply the loop's cell condition too, or the + // editor shows rows the published page will not. + const cellFilter = parseCellFilter({ + cellField: url.searchParams.get('cellField') ?? '', + cellOperator: url.searchParams.get('cellOperator') ?? '', + cellValue: url.searchParams.get('cellValue') ?? '', + }) + const result = await fetchPublishedDataRowItems(db, { tableId, orderBy, direction, limit, offset, + cellFilter, }) return jsonResponse(result) } diff --git a/src/__tests__/loops/cellFilter.test.ts b/src/__tests__/loops/cellFilter.test.ts new file mode 100644 index 000000000..ea874295e --- /dev/null +++ b/src/__tests__/loops/cellFilter.test.ts @@ -0,0 +1,165 @@ +/** + * Unit tests for the loop cell filter — the pure half. + * + * Two properties matter and both are easy to get wrong: + * - a half-configured filter must never silently empty a list, and + * - the SQL must bind BOTH the field name and the value, so a field id + * can never reach the statement text. + * + * The SQL/TypeScript predicates are also checked against each other: they + * are two spellings of one rule, and the canvas uses one while the + * publisher uses the other. + */ +import { describe, expect, test } from 'bun:test' +import { + cellFilterMatches, + cellFilterSql, + parseCellFilter, + CELL_FILTER_OPERATORS, + type CellFilter, +} from '@core/loops/cellFilter' + +describe('parseCellFilter', () => { + test('returns null when no field is chosen', () => { + expect(parseCellFilter({})).toBeNull() + expect(parseCellFilter({ cellField: ' ' })).toBeNull() + }) + + test('a comparison without a value is treated as not-yet-configured', () => { + expect(parseCellFilter({ cellField: 'featured', cellOperator: 'is' })).toBeNull() + expect(parseCellFilter({ cellField: 'featured', cellOperator: 'isNot', cellValue: '' })).toBeNull() + }) + + test('valueless operators need no value', () => { + expect(parseCellFilter({ cellField: 'featured', cellOperator: 'isTrue' })) + .toEqual({ field: 'featured', operator: 'isTrue', value: '' }) + }) + + test('defaults to `is` and coerces non-string values', () => { + expect(parseCellFilter({ cellField: 'rank', cellValue: 3 })) + .toEqual({ field: 'rank', operator: 'is', value: '3' }) + expect(parseCellFilter({ cellField: 'live', cellValue: true })) + .toEqual({ field: 'live', operator: 'is', value: 'true' }) + }) + + test('an unknown operator falls back to `is` rather than breaking the query', () => { + expect(parseCellFilter({ cellField: 'a', cellOperator: 'DROP TABLE', cellValue: 'x' })) + .toEqual({ field: 'a', operator: 'is', value: 'x' }) + }) +}) + +describe('cellFilterSql', () => { + const filter: CellFilter = { field: 'team-on-about-page', operator: 'isTrue', value: '' } + + test('binds the field name as a parameter — never as SQL text', () => { + for (const dialect of ['postgres', 'sqlite'] as const) { + const { sql, params } = cellFilterSql({ filter, dialect, column: 'data_rows.cells_json', nextParamIndex: 4 }) + expect(sql).not.toContain('team-on-about-page') + expect(params[0]).toBe('team-on-about-page') + } + }) + + test('a hostile field id cannot escape into the statement', () => { + const hostile: CellFilter = { field: "x'); drop table data_rows; --", operator: 'is', value: 'y' } + const { sql, params } = cellFilterSql({ filter: hostile, dialect: 'sqlite', column: 'c', nextParamIndex: 2 }) + expect(sql).not.toContain('drop table') + expect(params).toEqual([hostile.field, 'y']) + }) + + test('placeholders follow the dialect and start at the given index', () => { + const pg = cellFilterSql({ filter: { field: 'f', operator: 'is', value: 'v' }, dialect: 'postgres', column: 'c', nextParamIndex: 4 }) + expect(pg.sql).toContain('$4') + expect(pg.sql).toContain('$5') + const sqlite = cellFilterSql({ filter: { field: 'f', operator: 'is', value: 'v' }, dialect: 'sqlite', column: 'c', nextParamIndex: 4 }) + expect(sqlite.sql).toContain('?') + expect(sqlite.sql).not.toContain('$4') + }) + + test('every operator produces a fragment with the right parameter count', () => { + const cases: Array<[CellFilter['operator'], number]> = [ + ['is', 2], ['isNot', 2], ['isTrue', 1], ['isFalse', 1], ['isSet', 1], ['isEmpty', 1], + ] + for (const [operator, paramCount] of cases) { + const { sql, params } = cellFilterSql({ + filter: { field: 'f', operator, value: 'v' }, + dialect: 'sqlite', + column: 'c', + nextParamIndex: 1, + }) + expect(sql.length).toBeGreaterThan(0) + expect(params).toHaveLength(paramCount) + } + }) + + test('the JSON read appears once per fragment, so the field binds once', () => { + // Repeating the expression would repeat its placeholder while the caller + // binds the field name a single time — the bug that made SQLite reject + // the statement with "expected 3 values, received 2". + for (const operator of CELL_FILTER_OPERATORS) { + const { sql, params } = cellFilterSql({ + filter: { field: 'f', operator, value: 'v' }, + dialect: 'sqlite', + column: 'c', + nextParamIndex: 1, + }) + expect(sql.match(/json_extract/g) ?? []).toHaveLength(1) + expect(sql.match(/\?/g) ?? []).toHaveLength(params.length) + } + }) + + test('missing cells fold into the comparison instead of vanishing', () => { + // `coalesce(…, '')` is what keeps a row that never set the field inside + // "is not X" and "is unchecked". + for (const operator of ['isNot', 'isFalse', 'isEmpty'] as const) { + const { sql } = cellFilterSql({ + filter: { field: 'f', operator, value: 'v' }, + dialect: 'sqlite', + column: 'c', + nextParamIndex: 1, + }) + expect(sql).toContain('coalesce') + } + }) + + test('SQLite casts the JSON read so boolean cells compare as text', () => { + const { sql } = cellFilterSql({ filter: { field: 'f', operator: 'isTrue', value: '' }, dialect: 'sqlite', column: 'c', nextParamIndex: 1 }) + // Without the cast, json_extract returns INTEGER 1 and `1 = '1'` is false. + expect(sql).toContain('cast(') + expect(sql).toContain("'1'") + }) +}) + +describe('cellFilterMatches mirrors the SQL semantics', () => { + const rows = { + featuredTrue: { featured: true, name: 'A' }, + featuredFalse: { featured: false, name: 'B' }, + missing: { name: 'C' }, + empty: { featured: '', name: 'D' }, + } + + test('isTrue only matches a true value', () => { + const f: CellFilter = { field: 'featured', operator: 'isTrue', value: '' } + expect(cellFilterMatches(f, rows.featuredTrue)).toBe(true) + expect(cellFilterMatches(f, rows.featuredFalse)).toBe(false) + expect(cellFilterMatches(f, rows.missing)).toBe(false) + }) + + test('isFalse matches false AND a missing field', () => { + const f: CellFilter = { field: 'featured', operator: 'isFalse', value: '' } + expect(cellFilterMatches(f, rows.featuredFalse)).toBe(true) + expect(cellFilterMatches(f, rows.missing)).toBe(true) + expect(cellFilterMatches(f, rows.featuredTrue)).toBe(false) + }) + + test('is / isNot compare as text', () => { + expect(cellFilterMatches({ field: 'name', operator: 'is', value: 'A' }, rows.featuredTrue)).toBe(true) + expect(cellFilterMatches({ field: 'name', operator: 'isNot', value: 'A' }, rows.featuredTrue)).toBe(false) + expect(cellFilterMatches({ field: 'name', operator: 'isNot', value: 'A' }, rows.featuredFalse)).toBe(true) + }) + + test('isSet / isEmpty treat an empty string as empty', () => { + expect(cellFilterMatches({ field: 'featured', operator: 'isSet', value: '' }, rows.empty)).toBe(false) + expect(cellFilterMatches({ field: 'featured', operator: 'isEmpty', value: '' }, rows.empty)).toBe(true) + expect(cellFilterMatches({ field: 'featured', operator: 'isEmpty', value: '' }, rows.missing)).toBe(true) + }) +}) diff --git a/src/__tests__/loops/dataRowsCellFilter.test.ts b/src/__tests__/loops/dataRowsCellFilter.test.ts new file mode 100644 index 000000000..f06aafa74 --- /dev/null +++ b/src/__tests__/loops/dataRowsCellFilter.test.ts @@ -0,0 +1,165 @@ +/** + * Behavior tests for the `data.rows` loop cell filter against a real + * migrated SQLite database. + * + * The pure half (parsing, SQL assembly) is covered in `cellFilter.test.ts`; + * what matters here is that the condition actually reaches the query on + * BOTH table kinds, that `totalItems` counts the filtered set (otherwise + * pagination advertises rows the page query drops), and that a filter on a + * field some rows lack behaves the way an author expects. + */ + +import { describe, expect, it, beforeAll, afterAll } from 'bun:test' +import { createTestDb, type TestDb } from '../helpers/createTestDb' +import { fetchPublishedDataRowItems } from '@core/loops/sources/dataRows' +import type { CellFilter } from '@core/loops/cellFilter' + +type Db = TestDb['db'] + +let testDb: TestDb +let db: Db + +async function seedPost( + rowId: string, + slug: string, + cells: Record, + publishedAt: string, +): Promise { + await db` + insert into data_rows (id, table_id, cells_json, slug, status, updated_at) + values (${rowId}, ${'posts'}, ${JSON.stringify(cells)}, ${slug}, ${'published'}, ${publishedAt}) + ` + await db` + insert into data_row_versions (id, row_id, version_number, cells_json, slug, published_at, created_at) + values (${`${rowId}-v1`}, ${rowId}, ${1}, ${JSON.stringify(cells)}, ${slug}, ${publishedAt}, ${publishedAt}) + ` + await db`update data_rows set active_version_id = ${`${rowId}-v1`} where id = ${rowId}` +} + +async function seedDataRow( + tableId: string, + rowId: string, + slug: string, + cells: Record, +): Promise { + await db` + insert into data_rows (id, table_id, cells_json, slug, status, created_at, updated_at) + values (${rowId}, ${tableId}, ${JSON.stringify(cells)}, ${slug}, ${'draft'}, ${'2024-01-01T00:00:00Z'}, ${'2024-01-01T00:00:00Z'}) + ` +} + +async function slugsWith(tableId: string, cellFilter: CellFilter | null): Promise { + const { items } = await fetchPublishedDataRowItems(db, { + tableId, + orderBy: 'slug', + direction: 'asc', + limit: 50, + offset: 0, + cellFilter, + }) + return items.map((item) => String(item.fields['slug'])) +} + +beforeAll(async () => { + testDb = await createTestDb() + db = testDb.db + + // Post-type rows: two featured, one not, one missing the field entirely — + // the exact shape that made a real migration list the wrong three items. + await seedPost('p-a', 'alpha', { title: 'Alpha', featured: true, tag: 'news' }, '2024-01-01T00:00:00Z') + await seedPost('p-b', 'bravo', { title: 'Bravo', featured: false, tag: 'news' }, '2024-01-02T00:00:00Z') + await seedPost('p-c', 'charlie', { title: 'Charlie', featured: true, tag: 'guide' }, '2024-01-03T00:00:00Z') + await seedPost('p-d', 'delta', { title: 'Delta' }, '2024-01-04T00:00:00Z') + + await db` + insert into data_tables (id, name, slug, kind, route_base, singular_label, plural_label, fields_json, system) + values ('logos', 'Logos', 'logos', 'data', '/logos', 'Logo', 'Logos', ${JSON.stringify([])}, 0) + ` + await seedDataRow('logos', 'l-a', 'acme', { name: 'Acme', member: true }) + await seedDataRow('logos', 'l-b', 'globex', { name: 'Globex', member: false }) + await seedDataRow('logos', 'l-c', 'initech', { name: 'Initech' }) +}) + +afterAll(async () => { + await testDb.cleanup() +}) + +describe('data.rows cell filter — post-type tables', () => { + it('no filter lists every published row', async () => { + expect(await slugsWith('posts', null)).toEqual(['alpha', 'bravo', 'charlie', 'delta']) + }) + + it('isTrue keeps only the marked rows', async () => { + expect(await slugsWith('posts', { field: 'featured', operator: 'isTrue', value: '' })) + .toEqual(['alpha', 'charlie']) + }) + + it('isFalse includes rows that lack the field', async () => { + expect(await slugsWith('posts', { field: 'featured', operator: 'isFalse', value: '' })) + .toEqual(['bravo', 'delta']) + }) + + it('is matches a text cell exactly', async () => { + expect(await slugsWith('posts', { field: 'tag', operator: 'is', value: 'news' })) + .toEqual(['alpha', 'bravo']) + }) + + it('isNot also returns rows missing the field', async () => { + expect(await slugsWith('posts', { field: 'tag', operator: 'isNot', value: 'news' })) + .toEqual(['charlie', 'delta']) + }) + + it('isSet / isEmpty split on presence', async () => { + expect(await slugsWith('posts', { field: 'tag', operator: 'isSet', value: '' })) + .toEqual(['alpha', 'bravo', 'charlie']) + expect(await slugsWith('posts', { field: 'tag', operator: 'isEmpty', value: '' })) + .toEqual(['delta']) + }) + + it('totalItems counts the filtered set, not the table', async () => { + const { items, totalItems } = await fetchPublishedDataRowItems(db, { + tableId: 'posts', + orderBy: 'slug', + direction: 'asc', + limit: 1, + offset: 0, + cellFilter: { field: 'featured', operator: 'isTrue', value: '' }, + }) + expect(items).toHaveLength(1) + expect(totalItems).toBe(2) + }) + + it('paginates within the filtered set', async () => { + const { items } = await fetchPublishedDataRowItems(db, { + tableId: 'posts', + orderBy: 'slug', + direction: 'asc', + limit: 5, + offset: 1, + cellFilter: { field: 'featured', operator: 'isTrue', value: '' }, + }) + expect(items.map((i) => String(i.fields['slug']))).toEqual(['charlie']) + }) + + it('an unknown field matches nothing rather than everything', async () => { + expect(await slugsWith('posts', { field: 'nope', operator: 'isTrue', value: '' })).toEqual([]) + }) +}) + +describe('data.rows cell filter — data-kind tables', () => { + it('applies on the direct-read path too', async () => { + expect(await slugsWith('logos', { field: 'member', operator: 'isTrue', value: '' })).toEqual(['acme']) + }) + + it('counts the filtered set on the data-kind path', async () => { + const { totalItems } = await fetchPublishedDataRowItems(db, { + tableId: 'logos', + orderBy: 'slug', + direction: 'asc', + limit: 50, + offset: 0, + cellFilter: { field: 'member', operator: 'isFalse', value: '' }, + }) + expect(totalItems).toBe(2) + }) +}) diff --git a/src/admin/pages/site/canvas/useLoopPreviewItems.ts b/src/admin/pages/site/canvas/useLoopPreviewItems.ts index 8b920d7ef..ef70c11ba 100644 --- a/src/admin/pages/site/canvas/useLoopPreviewItems.ts +++ b/src/admin/pages/site/canvas/useLoopPreviewItems.ts @@ -254,6 +254,10 @@ export function useLoopPreviewItems( const { sourceId, filters, orderBy, direction, offset, limit } = readLoopProps(node) const tableId = typeof filters.tableId === 'string' ? filters.tableId : '' const mimePrefix = typeof filters.mimePrefix === 'string' ? filters.mimePrefix : '' + // Read as primitives so the fetch effect's dependency list stays stable. + const cellField = typeof filters.cellField === 'string' ? filters.cellField : '' + const cellOperator = typeof filters.cellOperator === 'string' ? filters.cellOperator : '' + const cellValue = typeof filters.cellValue === 'string' ? filters.cellValue : '' const isPluginSource = sourceId !== '' && !BUILT_IN_SOURCE_IDS.has(sourceId) // Narrow, identity-stable subscriptions (see module header). Inactive @@ -300,6 +304,9 @@ export function useLoopPreviewItems( direction, limit, offset, + cellField, + cellOperator, + cellValue, }) .then((result) => { if (!cancelled) setAsyncDataRowItems(result.items) @@ -311,7 +318,7 @@ export function useLoopPreviewItems( return () => { cancelled = true } - }, [sourceId, tableId, orderBy, direction, limit, offset, previewReadiness]) + }, [sourceId, tableId, orderBy, direction, limit, offset, cellField, cellOperator, cellValue, previewReadiness]) // ── Async fetch: site.media ───────────────────────────────────────── useEffect(() => { diff --git a/src/admin/pages/site/panels/PropertiesPanel/LoopPropertiesView.tsx b/src/admin/pages/site/panels/PropertiesPanel/LoopPropertiesView.tsx index 2859c4dfb..6b77edb4f 100644 --- a/src/admin/pages/site/panels/PropertiesPanel/LoopPropertiesView.tsx +++ b/src/admin/pages/site/panels/PropertiesPanel/LoopPropertiesView.tsx @@ -65,7 +65,13 @@ export function LoopPropertiesView({ nodeId, props, activePage }: LoopProperties if (source.id === 'data.rows' && tables) { const tableField = source.filterSchema.tableId if (tableField && tableField.type === 'select') { - return { + const selectedTable = tables.find((t) => t.id === filters.tableId) + const cellFieldControl = source.filterSchema.cellField + const operator = typeof filters.cellOperator === 'string' ? filters.cellOperator : 'is' + // The value box is meaningless for the checkbox / emptiness operators, + // and a stale value in it would read as a live condition. + const valuelessOperator = ['isTrue', 'isFalse', 'isSet', 'isEmpty'].includes(operator) + const schema: PropertySchema = { ...source.filterSchema, tableId: { ...tableField, @@ -75,6 +81,23 @@ export function LoopPropertiesView({ nodeId, props, activePage }: LoopProperties ], }, } + if (cellFieldControl?.type === 'select') { + schema.cellField = { + ...cellFieldControl, + options: [ + { label: '— every row —', value: '' }, + ...(selectedTable?.fields ?? []).map((f) => ({ label: f.label || f.id, value: f.id })), + ], + } + } + // Condition + value only matter once a field is picked. + if (!filters.cellField) { + delete schema.cellOperator + delete schema.cellValue + } else if (valuelessOperator) { + delete schema.cellValue + } + return schema } } if (source.id === ENTRY_FIELD_SOURCE_ID && tables) { diff --git a/src/core/loops/cellFilter.ts b/src/core/loops/cellFilter.ts new file mode 100644 index 000000000..51f1f1d7c --- /dev/null +++ b/src/core/loops/cellFilter.ts @@ -0,0 +1,148 @@ +/** + * Cell filtering for data-row loops. + * + * A loop could pick a table and an order, but not *which* rows — so a page + * that should list three featured articles listed the three most recent + * ones instead. This adds one condition on a row's own cell, which is what + * "featured", "show on homepage" or "category = X" style lists need. + * + * The value lives inside `cells_json`, so the comparison needs JSON access — + * the one place the two dialects genuinely differ. `cellFilterSql` isolates + * that behind the same `db.dialect` switch `positionalParam` already uses; + * everything else (parsing, validation, the closed operator set) is pure and + * unit-tested here. + * + * Deliberately ONE condition, not a query builder: it covers the real cases + * without inventing an AND/OR grammar the editor cannot express and future + * maintainers would have to keep sound. + */ + +/** Operators a loop filter can use. Closed set — never interpolated raw. */ +export const CELL_FILTER_OPERATORS = ['is', 'isNot', 'isTrue', 'isFalse', 'isSet', 'isEmpty'] as const + +export type CellFilterOperator = (typeof CELL_FILTER_OPERATORS)[number] + +export interface CellFilter { + /** Field id as stored in `cells_json` (a data-table field id). */ + field: string + operator: CellFilterOperator + /** Compared value for `is` / `isNot`; ignored by the other operators. */ + value: string +} + +/** Operators that ignore the comparison value. */ +const VALUELESS: ReadonlySet = new Set(['isTrue', 'isFalse', 'isSet', 'isEmpty']) + +export function isCellFilterOperator(value: unknown): value is CellFilterOperator { + return typeof value === 'string' && (CELL_FILTER_OPERATORS as readonly string[]).includes(value) +} + +/** + * Read a filter out of a loop's free-form `filters` bag. + * + * Returns null whenever the filter is absent or unusable, so a half-configured + * loop (field picked, operator not yet) keeps listing everything instead of + * silently returning nothing. + */ +export function parseCellFilter(filters: Record): CellFilter | null { + const field = typeof filters.cellField === 'string' ? filters.cellField.trim() : '' + if (!field) return null + + const operator: CellFilterOperator = isCellFilterOperator(filters.cellOperator) + ? filters.cellOperator + : 'is' + + const rawValue = filters.cellValue + const value = typeof rawValue === 'string' + ? rawValue.trim() + : typeof rawValue === 'number' || typeof rawValue === 'boolean' + ? String(rawValue) + : '' + + // `is` / `isNot` without a value would filter on the empty string, which is + // never what an author means — treat it as "not configured yet". + if (!VALUELESS.has(operator) && !value) return null + + return { field, operator, value } +} + +/** + * SQL fragment + parameters for a cell filter. + * + * `column` is the qualified JSON column (`data_rows.cells_json` or + * `data_row_versions.cells_json`). `nextParamIndex` is the 1-based index the + * first parameter of this fragment takes in the statement's parameter list; + * `placeholder` renders it in the dialect's own style. + * + * The field NAME is a parameter too — never string-concatenated into the SQL — + * so a crafted field id cannot escape into the statement. + */ +export function cellFilterSql(input: { + filter: CellFilter + dialect: 'postgres' | 'sqlite' + column: string + nextParamIndex: number +}): { sql: string; params: unknown[] } { + const { filter, dialect, column, nextParamIndex } = input + const placeholder = (offset: number) => + dialect === 'postgres' ? `$${nextParamIndex + offset}` : '?' + + // Postgres: `cells_json #>> array[key]` reads a text value at a dynamic key. + // SQLite: `json_extract(cells_json, '$.' || key)` does the same. Both take + // the key as a bound parameter. + // + // Two shapes matter here: + // - The expression appears EXACTLY ONCE per fragment. Repeating it would + // repeat its placeholder, and the caller binds the field name once. + // `coalesce(…, '')` folds the missing-field case into the comparison + // instead of needing a second `is null` branch. + // - Booleans do not read back identically: Postgres yields 'true'/'false' + // text, SQLite's json_extract yields the INTEGERS 1/0. SQLite compares + // across storage classes by class first, so `1 = '1'` is false — hence + // the cast, and hence the operators accepting both spellings. + const rawValue = dialect === 'postgres' + ? `(${column} #>> array[${placeholder(0)}])` + : `cast(json_extract(${column}, '$.' || ${placeholder(0)}) as text)` + const textValue = `coalesce(${rawValue}, '')` + + switch (filter.operator) { + case 'is': + return { sql: `${textValue} = ${placeholder(1)}`, params: [filter.field, filter.value] } + case 'isNot': + // A row missing the field is "not X" — the coalesce keeps it in. + return { sql: `${textValue} <> ${placeholder(1)}`, params: [filter.field, filter.value] } + case 'isTrue': + return { sql: `${textValue} in ('true', '1')`, params: [filter.field] } + case 'isFalse': + // Unchecked includes rows where the field was never set. + return { sql: `${textValue} in ('false', '0', '')`, params: [filter.field] } + case 'isSet': + return { sql: `${textValue} <> ''`, params: [filter.field] } + case 'isEmpty': + return { sql: `${textValue} = ''`, params: [filter.field] } + } +} + +/** + * The same predicate in TypeScript, for callers holding rows rather than a + * query — the canvas preview and any future in-memory path. Keeping it beside + * the SQL keeps the two definitions honest about each other. + */ +export function cellFilterMatches(filter: CellFilter, cells: Record): boolean { + const raw = cells[filter.field] + const text = raw === null || raw === undefined + ? null + : typeof raw === 'string' ? raw : typeof raw === 'number' || typeof raw === 'boolean' ? String(raw) : JSON.stringify(raw) + + // Mirrors the SQL exactly: a missing cell reads as the empty string, and + // the checked/unchecked operators accept both boolean spellings. + const value = text ?? '' + switch (filter.operator) { + case 'is': return value === filter.value + case 'isNot': return value !== filter.value + case 'isTrue': return value === 'true' || value === '1' + case 'isFalse': return value === 'false' || value === '0' || value === '' + case 'isSet': return value !== '' + case 'isEmpty': return value === '' + } +} diff --git a/src/core/loops/sources/dataRows.ts b/src/core/loops/sources/dataRows.ts index ab96d8151..4bccaacc6 100644 --- a/src/core/loops/sources/dataRows.ts +++ b/src/core/loops/sources/dataRows.ts @@ -20,6 +20,7 @@ */ import type { LoopEntitySource, LoopFetchResult, LoopItem, LoopSourceDb } from '@core/loops/types' +import { cellFilterSql, parseCellFilter, type CellFilter } from '../cellFilter' import { isoDate } from '../../utils/isoDate' import { firstImagePathFromMarkdown } from '@core/markdown/renderMarkdown' import { normalizeRouteBase } from '@core/templates/templateMatching' @@ -218,8 +219,18 @@ async function fetchPage( direction: 'asc' | 'desc', limit: number, offset: number, + filter: CellFilter | null, ): Promise { const orderColumn = POST_TYPE_ORDER_COLUMN[orderBy] + // SQLite binds `?` by POSITION IN THE TEXT, so the parameter list must follow + // the clause order: tableId, then the cell condition (WHERE), then + // limit/offset. Postgres indices are numbered to match. + const cell = filter + ? cellFilterSql({ filter, dialect: db.dialect, column: 'data_row_versions.cells_json', nextParamIndex: 2 }) + : null + const cellParams = cell?.params ?? [] + const limitParam = positionalParam(db, 2 + cellParams.length) + const offsetParam = positionalParam(db, 3 + cellParams.length) const { rows } = await db.unsafe( `select data_row_versions.id as version_id, data_rows.id as row_id, @@ -252,9 +263,10 @@ async function fetchPage( and data_rows.status = 'published' and data_rows.deleted_at is null and data_tables.deleted_at is null + ${cell ? `and ${cell.sql}` : ''} order by ${orderColumn} ${direction}, data_row_versions.id ${direction} - limit ${positionalParam(db, 2)} offset ${positionalParam(db, 3)}`, - [tableId, limit, offset], + limit ${limitParam} offset ${offsetParam}`, + [tableId, ...cellParams, limit, offset], ) return rows } @@ -357,10 +369,18 @@ async function fetchDataKindPage( direction: 'asc' | 'desc', limit: number, offset: number, + filter: CellFilter | null, ): Promise { const sortKey: 'createdAt' | 'updatedAt' | 'slug' = orderBy === 'updatedAt' ? 'updatedAt' : orderBy === 'slug' ? 'slug' : 'createdAt' const orderColumn = DATA_KIND_ORDER_COLUMN[sortKey] + // Parameter order follows the clause order — see `fetchPage`. + const cell = filter + ? cellFilterSql({ filter, dialect: db.dialect, column: 'data_rows.cells_json', nextParamIndex: 2 }) + : null + const cellParams = cell?.params ?? [] + const limitParam = positionalParam(db, 2 + cellParams.length) + const offsetParam = positionalParam(db, 3 + cellParams.length) // Same safety contract as `fetchPage`: the ORDER BY text comes only from // the closed map above; every runtime value is a positional parameter. @@ -384,9 +404,10 @@ async function fetchDataKindPage( where data_rows.table_id = ${positionalParam(db, 1)} and data_rows.deleted_at is null and data_tables.deleted_at is null + ${cell ? `and ${cell.sql}` : ''} order by ${orderColumn} ${direction}, data_rows.id ${direction} - limit ${positionalParam(db, 2)} offset ${positionalParam(db, 3)}`, - [tableId, limit, offset], + limit ${limitParam} offset ${offsetParam}`, + [tableId, ...cellParams, limit, offset], ) return rows } @@ -413,9 +434,12 @@ export async function fetchPublishedDataRowItems( direction: 'asc' | 'desc' limit: number offset: number + /** Optional condition on one of the row's own cells. */ + cellFilter?: CellFilter | null }, ): Promise { if (!opts.tableId) return { items: [], totalItems: 0 } + const cellFilter = opts.cellFilter ?? null const { rows: kindRows } = await db<{ kind: string }>` select kind @@ -433,17 +457,24 @@ export async function fetchPublishedDataRowItems( const direction: 'asc' | 'desc' = opts.direction === 'asc' ? 'asc' : 'desc' if (tableKind === 'data') { - const { rows: countRows } = await db<{ total: number }>` - select count(*) as total - from data_rows - where table_id = ${opts.tableId} - and deleted_at is null - ` + // The count must apply the same condition, or pagination advertises rows + // the page query filters out. + const dataCountCell = cellFilter + ? cellFilterSql({ filter: cellFilter, dialect: db.dialect, column: 'data_rows.cells_json', nextParamIndex: 2 }) + : null + const { rows: countRows } = await db.unsafe<{ total: number }>( + `select count(*) as total + from data_rows + where data_rows.table_id = ${positionalParam(db, 1)} + and data_rows.deleted_at is null + ${dataCountCell ? `and ${dataCountCell.sql}` : ''}`, + [opts.tableId, ...(dataCountCell?.params ?? [])], + ) const totalItems = Number(countRows[0]?.total ?? 0) if (totalItems === 0) return { items: [], totalItems: 0 } const sqlRows = await fetchDataKindPage( - db, opts.tableId, orderBy, direction, opts.limit, opts.offset, + db, opts.tableId, orderBy, direction, opts.limit, opts.offset, cellFilter, ) const mediaPathMap = await resolveMediaIdsToPaths(db, extractFeaturedMediaIds(sqlRows)) return { @@ -453,18 +484,23 @@ export async function fetchPublishedDataRowItems( } // Post-type path (default): only published rows, joined to active version. - const { rows: countRows } = await db<{ total: number }>` - select count(*) as total - from data_rows - join data_row_versions on data_row_versions.id = data_rows.active_version_id - where data_rows.table_id = ${opts.tableId} - and data_rows.status = 'published' - and data_rows.deleted_at is null - ` + const postCountCell = cellFilter + ? cellFilterSql({ filter: cellFilter, dialect: db.dialect, column: 'data_row_versions.cells_json', nextParamIndex: 2 }) + : null + const { rows: countRows } = await db.unsafe<{ total: number }>( + `select count(*) as total + from data_rows + join data_row_versions on data_row_versions.id = data_rows.active_version_id + where data_rows.table_id = ${positionalParam(db, 1)} + and data_rows.status = 'published' + and data_rows.deleted_at is null + ${postCountCell ? `and ${postCountCell.sql}` : ''}`, + [opts.tableId, ...(postCountCell?.params ?? [])], + ) const totalItems = Number(countRows[0]?.total ?? 0) if (totalItems === 0) return { items: [], totalItems: 0 } - const sqlRows = await fetchPage(db, opts.tableId, orderBy, direction, opts.limit, opts.offset) + const sqlRows = await fetchPage(db, opts.tableId, orderBy, direction, opts.limit, opts.offset, cellFilter) const mediaPathMap = await resolveMediaIdsToPaths(db, extractFeaturedMediaIds(sqlRows)) return { @@ -491,6 +527,30 @@ export const DataRowsSource: LoopEntitySource = { // valid when the source is registered before the table list is loaded. options: [], }, + // Optional condition on one of the row's own cells: the difference + // between "the newest three" and "the three marked featured". Field + // options are populated per selected table by the Properties Panel. + cellField: { + type: 'select', + label: 'Only rows where', + options: [], + }, + cellOperator: { + type: 'select', + label: 'Condition', + options: [ + { label: 'is', value: 'is' }, + { label: 'is not', value: 'isNot' }, + { label: 'is checked', value: 'isTrue' }, + { label: 'is unchecked', value: 'isFalse' }, + { label: 'has any value', value: 'isSet' }, + { label: 'is empty', value: 'isEmpty' }, + ], + }, + cellValue: { + type: 'text', + label: 'Value', + }, }, orderByOptions: [ @@ -526,6 +586,7 @@ export const DataRowsSource: LoopEntitySource = { direction: ctx.direction, limit: ctx.limit, offset: ctx.offset, + cellFilter: parseCellFilter(ctx.filters), }) }, diff --git a/src/core/persistence/cmsData.ts b/src/core/persistence/cmsData.ts index b7626b44b..7d755df86 100644 --- a/src/core/persistence/cmsData.ts +++ b/src/core/persistence/cmsData.ts @@ -365,6 +365,10 @@ interface DataLoopPreviewOptions { direction?: 'asc' | 'desc' limit?: number offset?: number + /** Cell condition, so the canvas previews the rows the page will publish. */ + cellField?: string + cellOperator?: string + cellValue?: string } interface DataLoopPreviewResult { @@ -386,6 +390,9 @@ export async function previewCmsDataLoopItems( direction: options.direction, limit: options.limit, offset: options.offset, + cellField: options.cellField, + cellOperator: options.cellOperator, + cellValue: options.cellValue, }, schema: LoopPreviewEnvelope, fetchImpl, From b3509e88dc64833c50c13554b0b29102f5194ed3 Mon Sep 17 00:00:00 2001 From: Mostafa Sadeghi <205455727+mostafasadeghidev@users.noreply.github.com> Date: Thu, 6 Aug 2026 12:36:03 +0200 Subject: [PATCH 2/5] feat(loops): sort data-row loops by a cell value MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Ordering was limited to the row's own SQL columns, so a list could not be sorted by a real date, title, or rank that lives in cells_json. A migrated site could only approximate 'newest first' with import arrival order, which drifts the moment anything is re-imported. - `orderBy` now also accepts `cell:`; `parseCellOrder` reads it and `cellOrderSql` renders the expression with the field name bound as a parameter, so nothing reaches the SQL text. - Both query paths order by it, values compare as TEXT in both dialects (ISO dates sort chronologically; documented that numbers sort lexicographically — one predictable rule beats two engine-specific ones), and coalesce gives rows lacking the field a defined position. - The Loop panel lists the selected table's fields as order options. Riding on `orderBy` rather than a new prop means every caller that already threads it — publisher, canvas preview, imported data-order-by attributes — supports this without further plumbing. Co-Authored-By: Claude Fable 5 --- src/__tests__/loops/cellFilter.test.ts | 35 +++++++++ .../loops/dataRowsCellFilter.test.ts | 71 +++++++++++++++++- .../PropertiesPanel/LoopPropertiesView.tsx | 18 +++-- src/core/loops/cellFilter.ts | 47 +++++++++++- src/core/loops/sources/dataRows.ts | 73 ++++++++++++------- 5 files changed, 209 insertions(+), 35 deletions(-) diff --git a/src/__tests__/loops/cellFilter.test.ts b/src/__tests__/loops/cellFilter.test.ts index ea874295e..50a7f3953 100644 --- a/src/__tests__/loops/cellFilter.test.ts +++ b/src/__tests__/loops/cellFilter.test.ts @@ -14,7 +14,9 @@ import { describe, expect, test } from 'bun:test' import { cellFilterMatches, cellFilterSql, + cellOrderSql, parseCellFilter, + parseCellOrder, CELL_FILTER_OPERATORS, type CellFilter, } from '@core/loops/cellFilter' @@ -129,6 +131,39 @@ describe('cellFilterSql', () => { }) }) +describe('parseCellOrder', () => { + test('only `cell:` values mean a cell sort', () => { + expect(parseCellOrder('publishedAt')).toBeNull() + expect(parseCellOrder('')).toBeNull() + expect(parseCellOrder('cell:published-on')).toEqual({ field: 'published-on' }) + }) + + test('a prefix with no field is not a sort', () => { + expect(parseCellOrder('cell:')).toBeNull() + expect(parseCellOrder('cell: ')).toBeNull() + }) +}) + +describe('cellOrderSql', () => { + test('binds the field name and never writes it into the SQL', () => { + for (const dialect of ['postgres', 'sqlite'] as const) { + const { sql, params } = cellOrderSql({ field: 'published-on', dialect, column: 'c', paramIndex: 2 }) + expect(sql).not.toContain('published-on') + expect(params).toEqual(['published-on']) + } + }) + + test('rows without the field get a defined sort position', () => { + const { sql } = cellOrderSql({ field: 'f', dialect: 'sqlite', column: 'c', paramIndex: 1 }) + expect(sql).toContain('coalesce') + }) + + test('placeholder style follows the dialect', () => { + expect(cellOrderSql({ field: 'f', dialect: 'postgres', column: 'c', paramIndex: 3 }).sql).toContain('$3') + expect(cellOrderSql({ field: 'f', dialect: 'sqlite', column: 'c', paramIndex: 3 }).sql).toContain('?') + }) +}) + describe('cellFilterMatches mirrors the SQL semantics', () => { const rows = { featuredTrue: { featured: true, name: 'A' }, diff --git a/src/__tests__/loops/dataRowsCellFilter.test.ts b/src/__tests__/loops/dataRowsCellFilter.test.ts index f06aafa74..3be672969 100644 --- a/src/__tests__/loops/dataRowsCellFilter.test.ts +++ b/src/__tests__/loops/dataRowsCellFilter.test.ts @@ -66,10 +66,12 @@ beforeAll(async () => { // Post-type rows: two featured, one not, one missing the field entirely — // the exact shape that made a real migration list the wrong three items. - await seedPost('p-a', 'alpha', { title: 'Alpha', featured: true, tag: 'news' }, '2024-01-01T00:00:00Z') - await seedPost('p-b', 'bravo', { title: 'Bravo', featured: false, tag: 'news' }, '2024-01-02T00:00:00Z') - await seedPost('p-c', 'charlie', { title: 'Charlie', featured: true, tag: 'guide' }, '2024-01-03T00:00:00Z') - await seedPost('p-d', 'delta', { title: 'Delta' }, '2024-01-04T00:00:00Z') + // `published-on` deliberately disagrees with the row's own publish column, + // so a cell sort cannot be mistaken for a column sort. + await seedPost('p-a', 'alpha', { title: 'Alpha', featured: true, tag: 'news', 'published-on': '2023-05-02' }, '2024-01-01T00:00:00Z') + await seedPost('p-b', 'bravo', { title: 'Bravo', featured: false, tag: 'news', 'published-on': '2023-09-30' }, '2024-01-02T00:00:00Z') + await seedPost('p-c', 'charlie', { title: 'Charlie', featured: true, tag: 'guide', 'published-on': '2023-01-15' }, '2024-01-03T00:00:00Z') + await seedPost('p-d', 'delta', { title: 'Delta', 'published-on': '2023-07-11' }, '2024-01-04T00:00:00Z') await db` insert into data_tables (id, name, slug, kind, route_base, singular_label, plural_label, fields_json, system) @@ -146,6 +148,67 @@ describe('data.rows cell filter — post-type tables', () => { }) }) +describe('data.rows ordering by a cell', () => { + it('sorts by the cell, not by the row columns', async () => { + // Seed order is alpha, bravo, charlie, delta; the dates deliberately + // disagree with it so a column sort cannot produce this result. + const { items } = await fetchPublishedDataRowItems(db, { + tableId: 'posts', + orderBy: 'cell:published-on', + direction: 'desc', + limit: 10, + offset: 0, + }) + expect(items.map((i) => String(i.fields['slug']))).toEqual(['bravo', 'delta', 'alpha', 'charlie']) + }) + + it('reverses cleanly', async () => { + const { items } = await fetchPublishedDataRowItems(db, { + tableId: 'posts', + orderBy: 'cell:published-on', + direction: 'asc', + limit: 10, + offset: 0, + }) + expect(items.map((i) => String(i.fields['slug']))).toEqual(['charlie', 'alpha', 'delta', 'bravo']) + }) + + it('combines with a filter and keeps the filtered count', async () => { + const { items, totalItems } = await fetchPublishedDataRowItems(db, { + tableId: 'posts', + orderBy: 'cell:published-on', + direction: 'desc', + limit: 10, + offset: 0, + cellFilter: { field: 'featured', operator: 'isTrue', value: '' }, + }) + expect(items.map((i) => String(i.fields['slug']))).toEqual(['alpha', 'charlie']) + expect(totalItems).toBe(2) + }) + + it('works on the data-kind path too', async () => { + const { items } = await fetchPublishedDataRowItems(db, { + tableId: 'logos', + orderBy: 'cell:name', + direction: 'desc', + limit: 10, + offset: 0, + }) + expect(items.map((i) => String(i.fields['slug']))).toEqual(['initech', 'globex', 'acme']) + }) + + it('an unknown sort field leaves every row present', async () => { + const { items } = await fetchPublishedDataRowItems(db, { + tableId: 'posts', + orderBy: 'cell:does-not-exist', + direction: 'desc', + limit: 10, + offset: 0, + }) + expect(items).toHaveLength(4) + }) +}) + describe('data.rows cell filter — data-kind tables', () => { it('applies on the direct-read path too', async () => { expect(await slugsWith('logos', { field: 'member', operator: 'isTrue', value: '' })).toEqual(['acme']) diff --git a/src/admin/pages/site/panels/PropertiesPanel/LoopPropertiesView.tsx b/src/admin/pages/site/panels/PropertiesPanel/LoopPropertiesView.tsx index 6b77edb4f..a9d77c2a0 100644 --- a/src/admin/pages/site/panels/PropertiesPanel/LoopPropertiesView.tsx +++ b/src/admin/pages/site/panels/PropertiesPanel/LoopPropertiesView.tsx @@ -17,6 +17,7 @@ import { useAsyncResource } from '@admin/lib/useAsyncResource' import { useEditorStore } from '@site/store/store' import { loopSourceRegistry } from '@core/loops/registry' import { ENTRY_FIELD_FILTER_KEY, ENTRY_FIELD_SOURCE_ID } from '@core/loops' +import { CELL_ORDER_PREFIX } from '@core/loops/cellFilter' import type { LoopEntitySource } from '@core/loops/types' import type { DataTableListItem } from '@core/data/schemas' import type { PropertyControl, PropertySchema } from '@core/module-engine' @@ -119,14 +120,21 @@ export function LoopPropertiesView({ nodeId, props, activePage }: LoopProperties } const filterSchema = buildFilterSchema() - // Order options reactive to source change. + // Order options reactive to source change. For data rows the selected + // table's own fields are offered too (`cell:`), so a list can sort by a + // real date or title instead of only by the row's SQL columns. const orderOptions: PropertyControl = { type: 'select', label: 'Order by', - options: - source?.orderByOptions.map((o) => ({ label: o.label, value: o.id })) ?? [ - { label: 'Default', value: '' }, - ], + options: source + ? [ + ...source.orderByOptions.map((o) => ({ label: o.label, value: o.id })), + ...(source.id === 'data.rows' + ? (tables?.find((t) => t.id === filters.tableId)?.fields ?? []) + .map((f) => ({ label: `${f.label || f.id} (field)`, value: `${CELL_ORDER_PREFIX}${f.id}` })) + : []), + ] + : [{ label: 'Default', value: '' }], } function handleSourceChange(_key: string, value: unknown) { diff --git a/src/core/loops/cellFilter.ts b/src/core/loops/cellFilter.ts index 51f1f1d7c..aba88a29d 100644 --- a/src/core/loops/cellFilter.ts +++ b/src/core/loops/cellFilter.ts @@ -1,5 +1,6 @@ /** - * Cell filtering for data-row loops. + * Cell access for data-row loops — filtering and ordering by a row's own + * cell rather than only by the table's SQL columns. * * A loop could pick a table and an order, but not *which* rows — so a page * that should list three featured articles listed the three most recent @@ -17,6 +18,50 @@ * maintainers would have to keep sound. */ +// --------------------------------------------------------------------------- +// Ordering by a cell +// --------------------------------------------------------------------------- + +/** `orderBy` values of this shape sort by a cell instead of a column. */ +export const CELL_ORDER_PREFIX = 'cell:' + +/** + * Read a cell-ordering request out of a loop's `orderBy`. + * + * Riding on `orderBy` (rather than a second prop) keeps ordering in one + * place: callers that already thread `orderBy` — the publisher, the canvas + * preview endpoint, imported `data-order-by` attributes — get this for free. + */ +export function parseCellOrder(orderBy: string): { field: string } | null { + if (!orderBy.startsWith(CELL_ORDER_PREFIX)) return null + const field = orderBy.slice(CELL_ORDER_PREFIX.length).trim() + return field ? { field } : null +} + +/** + * `ORDER BY` expression for a cell, with the field name bound as a parameter. + * + * Values are compared as TEXT in both dialects. ISO dates — the reason this + * exists — sort chronologically that way, and text sorts naturally. Numbers + * sort lexicographically (`'10' < '9'`), which is the price of one predictable + * rule across Postgres and SQLite instead of two subtly different ones. + */ +export function cellOrderSql(input: { + field: string + dialect: 'postgres' | 'sqlite' + column: string + paramIndex: number +}): { sql: string; params: unknown[] } { + const { field, dialect, column, paramIndex } = input + const placeholder = dialect === 'postgres' ? `$${paramIndex}` : '?' + const raw = dialect === 'postgres' + ? `(${column} #>> array[${placeholder}])` + : `cast(json_extract(${column}, '$.' || ${placeholder}) as text)` + // `coalesce` keeps rows that lack the field in one predictable place instead + // of relying on NULL ordering, which differs between the engines. + return { sql: `coalesce(${raw}, '')`, params: [field] } +} + /** Operators a loop filter can use. Closed set — never interpolated raw. */ export const CELL_FILTER_OPERATORS = ['is', 'isNot', 'isTrue', 'isFalse', 'isSet', 'isEmpty'] as const diff --git a/src/core/loops/sources/dataRows.ts b/src/core/loops/sources/dataRows.ts index 4bccaacc6..41d7083f6 100644 --- a/src/core/loops/sources/dataRows.ts +++ b/src/core/loops/sources/dataRows.ts @@ -20,7 +20,7 @@ */ import type { LoopEntitySource, LoopFetchResult, LoopItem, LoopSourceDb } from '@core/loops/types' -import { cellFilterSql, parseCellFilter, type CellFilter } from '../cellFilter' +import { cellFilterSql, cellOrderSql, parseCellFilter, parseCellOrder, type CellFilter } from '../cellFilter' import { isoDate } from '../../utils/isoDate' import { firstImagePathFromMarkdown } from '@core/markdown/renderMarkdown' import { normalizeRouteBase } from '@core/templates/templateMatching' @@ -214,23 +214,27 @@ const POST_TYPE_ORDER_COLUMN: Record = { async function fetchPage( db: LoopSourceDb, - tableId: string, orderBy: OrderColumn, direction: 'asc' | 'desc', - limit: number, - offset: number, - filter: CellFilter | null, + opts: { tableId: string; limit: number; offset: number; filter: CellFilter | null; orderCellField: string | null }, ): Promise { - const orderColumn = POST_TYPE_ORDER_COLUMN[orderBy] + const { tableId, limit, offset, filter, orderCellField } = opts + const column = 'data_row_versions.cells_json' // SQLite binds `?` by POSITION IN THE TEXT, so the parameter list must follow - // the clause order: tableId, then the cell condition (WHERE), then - // limit/offset. Postgres indices are numbered to match. + // the clause order: tableId, the cell condition (WHERE), the ordering cell + // (ORDER BY), then limit/offset. Postgres indices are numbered to match. const cell = filter - ? cellFilterSql({ filter, dialect: db.dialect, column: 'data_row_versions.cells_json', nextParamIndex: 2 }) + ? cellFilterSql({ filter, dialect: db.dialect, column, nextParamIndex: 2 }) : null const cellParams = cell?.params ?? [] - const limitParam = positionalParam(db, 2 + cellParams.length) - const offsetParam = positionalParam(db, 3 + cellParams.length) + const order = orderCellField + ? cellOrderSql({ field: orderCellField, dialect: db.dialect, column, paramIndex: 2 + cellParams.length }) + : null + const orderColumn = order ? order.sql : POST_TYPE_ORDER_COLUMN[orderBy] + const orderParams = order?.params ?? [] + const before = cellParams.length + orderParams.length + const limitParam = positionalParam(db, 2 + before) + const offsetParam = positionalParam(db, 3 + before) const { rows } = await db.unsafe( `select data_row_versions.id as version_id, data_rows.id as row_id, @@ -266,7 +270,7 @@ async function fetchPage( ${cell ? `and ${cell.sql}` : ''} order by ${orderColumn} ${direction}, data_row_versions.id ${direction} limit ${limitParam} offset ${offsetParam}`, - [tableId, ...cellParams, limit, offset], + [tableId, ...cellParams, ...orderParams, limit, offset], ) return rows } @@ -364,23 +368,27 @@ const DATA_KIND_ORDER_COLUMN: Record<'createdAt' | 'updatedAt' | 'slug', string> async function fetchDataKindPage( db: LoopSourceDb, - tableId: string, orderBy: OrderColumn, direction: 'asc' | 'desc', - limit: number, - offset: number, - filter: CellFilter | null, + opts: { tableId: string; limit: number; offset: number; filter: CellFilter | null; orderCellField: string | null }, ): Promise { + const { tableId, limit, offset, filter, orderCellField } = opts const sortKey: 'createdAt' | 'updatedAt' | 'slug' = orderBy === 'updatedAt' ? 'updatedAt' : orderBy === 'slug' ? 'slug' : 'createdAt' - const orderColumn = DATA_KIND_ORDER_COLUMN[sortKey] + const column = 'data_rows.cells_json' // Parameter order follows the clause order — see `fetchPage`. const cell = filter - ? cellFilterSql({ filter, dialect: db.dialect, column: 'data_rows.cells_json', nextParamIndex: 2 }) + ? cellFilterSql({ filter, dialect: db.dialect, column, nextParamIndex: 2 }) : null const cellParams = cell?.params ?? [] - const limitParam = positionalParam(db, 2 + cellParams.length) - const offsetParam = positionalParam(db, 3 + cellParams.length) + const order = orderCellField + ? cellOrderSql({ field: orderCellField, dialect: db.dialect, column, paramIndex: 2 + cellParams.length }) + : null + const orderColumn = order ? order.sql : DATA_KIND_ORDER_COLUMN[sortKey] + const orderParams = order?.params ?? [] + const before = cellParams.length + orderParams.length + const limitParam = positionalParam(db, 2 + before) + const offsetParam = positionalParam(db, 3 + before) // Same safety contract as `fetchPage`: the ORDER BY text comes only from // the closed map above; every runtime value is a positional parameter. @@ -407,7 +415,7 @@ async function fetchDataKindPage( ${cell ? `and ${cell.sql}` : ''} order by ${orderColumn} ${direction}, data_rows.id ${direction} limit ${limitParam} offset ${offsetParam}`, - [tableId, ...cellParams, limit, offset], + [tableId, ...cellParams, ...orderParams, limit, offset], ) return rows } @@ -451,6 +459,11 @@ export async function fetchPublishedDataRowItems( const tableKind = kindRows[0]?.kind if (!tableKind) return { items: [], totalItems: 0 } + // `orderBy` is either one of the whitelisted columns or `cell:`, + // in which case the sort runs on the row's own cell (the field name binds + // as a parameter, so nothing reaches the SQL text). + const cellOrder = parseCellOrder(opts.orderBy) + const orderCellField = cellOrder?.field ?? null const orderBy: OrderColumn = ALLOWED_ORDER_BY.has(opts.orderBy as OrderColumn) ? (opts.orderBy as OrderColumn) : 'publishedAt' @@ -473,9 +486,13 @@ export async function fetchPublishedDataRowItems( const totalItems = Number(countRows[0]?.total ?? 0) if (totalItems === 0) return { items: [], totalItems: 0 } - const sqlRows = await fetchDataKindPage( - db, opts.tableId, orderBy, direction, opts.limit, opts.offset, cellFilter, - ) + const sqlRows = await fetchDataKindPage(db, orderBy, direction, { + tableId: opts.tableId, + limit: opts.limit, + offset: opts.offset, + filter: cellFilter, + orderCellField, + }) const mediaPathMap = await resolveMediaIdsToPaths(db, extractFeaturedMediaIds(sqlRows)) return { items: sqlRows.map((row) => dataKindRowToLoopItem(row, mediaPathMap)), @@ -500,7 +517,13 @@ export async function fetchPublishedDataRowItems( const totalItems = Number(countRows[0]?.total ?? 0) if (totalItems === 0) return { items: [], totalItems: 0 } - const sqlRows = await fetchPage(db, opts.tableId, orderBy, direction, opts.limit, opts.offset, cellFilter) + const sqlRows = await fetchPage(db, orderBy, direction, { + tableId: opts.tableId, + limit: opts.limit, + offset: opts.offset, + filter: cellFilter, + orderCellField, + }) const mediaPathMap = await resolveMediaIdsToPaths(db, extractFeaturedMediaIds(sqlRows)) return { From c0f932fd34d8429b4dfabe853652ee43ab75479d Mon Sep 17 00:00:00 2001 From: Mostafa Sadeghi <205455727+mostafasadeghidev@users.noreply.github.com> Date: Thu, 6 Aug 2026 13:03:05 +0200 Subject: [PATCH 3/5] fix(loops): label the cell filter's presence operator "is set" "has any value" described the same condition in more words. The operator id was already `isSet`; the label now says so too. Co-Authored-By: Claude Fable 5 --- src/core/loops/sources/dataRows.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/core/loops/sources/dataRows.ts b/src/core/loops/sources/dataRows.ts index 41d7083f6..e2e93e763 100644 --- a/src/core/loops/sources/dataRows.ts +++ b/src/core/loops/sources/dataRows.ts @@ -566,7 +566,7 @@ export const DataRowsSource: LoopEntitySource = { { label: 'is not', value: 'isNot' }, { label: 'is checked', value: 'isTrue' }, { label: 'is unchecked', value: 'isFalse' }, - { label: 'has any value', value: 'isSet' }, + { label: 'is set', value: 'isSet' }, { label: 'is empty', value: 'isEmpty' }, ], }, From e72499e2e153b65274ad957cfd6fee9bb914b374 Mon Sep 17 00:00:00 2001 From: Mostafa Sadeghi <205455727+mostafasadeghidev@users.noreply.github.com> Date: Mon, 17 Aug 2026 18:28:56 +0200 Subject: [PATCH 4/5] refactor(loops): lift data-row media resolution into its own module MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Merging upstream put this branch's filter/order plumbing and #300's wider media resolution into the same file, and the result crossed the 700-line module ceiling at 711 — CI caught it. Raising the ceiling would have been the wrong repair. The two halves answer different questions: the rest of the file decides WHICH rows a loop returns — the filter, the order, the page window — while this block decides what a row's media cells CONTAIN once those rows are in hand. They only ever met because both needed the same page of rows. So `resolveMediaIdsToPaths`, `collectMediaIds` and `resolvedMediaOverlay` move to `dataRowsMedia.ts` unchanged, and the source file imports them. No behaviour changes: 596 lines and 132 rather than 711. `MediaAssetRow` and the `readMediaCellIds` / `readRepeaterCell` imports went with them, and the one test that reached for `resolveMediaIdsToPaths` now imports it from where it lives. Co-Authored-By: Claude Opus 5 --- .../server/mediaBatchResolution.test.ts | 4 +- src/core/loops/sources/dataRows.ts | 121 +--------------- src/core/loops/sources/dataRowsMedia.ts | 132 ++++++++++++++++++ 3 files changed, 137 insertions(+), 120 deletions(-) create mode 100644 src/core/loops/sources/dataRowsMedia.ts diff --git a/src/__tests__/server/mediaBatchResolution.test.ts b/src/__tests__/server/mediaBatchResolution.test.ts index 7c0a08bba..a1498e422 100644 --- a/src/__tests__/server/mediaBatchResolution.test.ts +++ b/src/__tests__/server/mediaBatchResolution.test.ts @@ -1,7 +1,7 @@ /** * Focused tests for the batched media-resolution helpers. * - * Finding 1 — resolveMediaIdsToPaths (src/core/loops/sources/dataRows.ts): + * Finding 1 — resolveMediaIdsToPaths (src/core/loops/sources/dataRowsMedia.ts): * Verifies that N media-id lookups collapse into ONE query (not N), that * repeated ids are deduplicated before the query, and that ids absent from * the database are absent from the returned map. @@ -18,7 +18,7 @@ import { describe, expect, it } from 'bun:test' import { createTestDb } from '../helpers/createTestDb' import { createFakeDb } from './dbTestFake' -import { resolveMediaIdsToPaths } from '../../../src/core/loops/sources/dataRows' +import { resolveMediaIdsToPaths } from '../../../src/core/loops/sources/dataRowsMedia' import { prefetchMediaAssets } from '../../../server/publish/mediaPrefetch' import type { IModuleRegistry } from '../../../src/core/module-engine' diff --git a/src/core/loops/sources/dataRows.ts b/src/core/loops/sources/dataRows.ts index f443f4791..4cf3dbc8e 100644 --- a/src/core/loops/sources/dataRows.ts +++ b/src/core/loops/sources/dataRows.ts @@ -26,8 +26,9 @@ import { firstImagePathFromMarkdown } from '@core/markdown/renderMarkdown' import { normalizeRouteBase } from '@core/templates/templateMatching' import { publicDataUserFromParts } from '@core/data/publicDataUser' import { normalizeDataTableFields } from '@core/data/fields' -import { readFeaturedMediaCell, readMediaCellIds, readRepeaterCell } from '@core/data/cells' -import type { DataField, DataRowCells, RepeaterItemField } from '@core/data/schemas' +import { readFeaturedMediaCell } from '@core/data/cells' +import type { DataField, DataRowCells } from '@core/data/schemas' +import { collectMediaIds, resolveMediaIdsToPaths, resolvedMediaOverlay } from './dataRowsMedia' // --------------------------------------------------------------------------- // Internal SQL row shape @@ -56,11 +57,6 @@ interface PublishedDataRowSqlRow { updated_at: Date | string } -interface MediaAssetRow { - id: string - public_path: string -} - interface DataTableProjectionRow { kind: string fields_json: unknown @@ -87,117 +83,6 @@ function positionalParam(db: LoopSourceDb, index: number): string { return db.dialect === 'postgres' ? `$${index}` : '?' } -// --------------------------------------------------------------------------- -// Media path resolution -// -// Media ids live inside cells_json, not as SQL columns — the built-in -// `featuredMedia` cell plus every user-defined `media` field. We extract the -// ids from each row's cells in TypeScript, deduplicate the set, and resolve -// all unique ids with a SINGLE batched IN-query. One round trip regardless of -// how many rows (or media fields) the page slice returned. -// --------------------------------------------------------------------------- - -/** - * Resolve a set of media asset ids to their public_path values in one query. - * Uses db.unsafe with dialect-appropriate positional placeholders so the - * same code works on both Postgres ($1, $2, …) and SQLite (?, ?, …). - * Ids absent from the database are absent from the returned map. - */ -export async function resolveMediaIdsToPaths( - db: LoopSourceDb, - ids: Iterable, -): Promise> { - const idList = [...new Set(ids)] - const pathMap = new Map() - if (idList.length === 0) return pathMap - const placeholders = idList.map((_, i) => positionalParam(db, i + 1)).join(', ') - const { rows } = await db.unsafe( - `select id, public_path - from media_assets - where id in (${placeholders}) and deleted_at is null`, - idList, - ) - for (const row of rows) pathMap.set(row.id, row.public_path) - return pathMap -} - -type MediaProjectionField = DataField | RepeaterItemField - -function collectFieldMediaIds( - cells: DataRowCells, - fields: readonly MediaProjectionField[], - ids: string[], -): void { - for (const field of fields) { - if (field.type === 'media') { - ids.push(...readMediaCellIds(cells, field.id)) - continue - } - if (field.type !== 'repeater') continue - for (const item of readRepeaterCell(cells, field.id)) { - collectFieldMediaIds(item.cells, field.fields, ids) - } - } -} - -/** - * Collect every media id referenced by a page of rows: the built-in - * `featuredMedia` cell plus every schema-declared media field. Repeater media - * is traversed recursively, and multi-value cells contribute every id while - * still resolving through one batched query. - */ -function collectMediaIds( - rows: Array<{ cells_json: Record }>, - fields: readonly DataField[], -): string[] { - const ids: string[] = [] - for (const row of rows) { - const cells = row.cells_json as DataRowCells - const featured = readFeaturedMediaCell(cells) - if (featured) ids.push(featured) - collectFieldMediaIds(cells, fields, ids) - } - return ids -} - -/** - * Resolve schema-declared media ids without changing collection cardinality: - * scalar media becomes a public path (or null), multi-media stays an ordered - * array of resolvable public paths, and repeater items keep their `{ id, cells }` - * shape while media inside `cells` is projected recursively. - */ -function resolvedMediaOverlay( - cells: DataRowCells, - fields: readonly MediaProjectionField[], - mediaPathMap: Map, -): DataRowCells { - const overlay: DataRowCells = {} - for (const field of fields) { - if (field.type === 'media') { - const ids = readMediaCellIds(cells, field.id) - if (field.allowMultiple === true) { - overlay[field.id] = ids.flatMap((id) => { - const path = mediaPathMap.get(id) - return path ? [path] : [] - }) - } else { - const id = ids[0] - overlay[field.id] = id ? (mediaPathMap.get(id) ?? null) : null - } - continue - } - if (field.type !== 'repeater') continue - overlay[field.id] = readRepeaterCell(cells, field.id).map((item) => ({ - ...item, - cells: { - ...item.cells, - ...resolvedMediaOverlay(item.cells, field.fields, mediaPathMap), - }, - })) - } - return overlay -} - // --------------------------------------------------------------------------- // Row → LoopItem projection // --------------------------------------------------------------------------- diff --git a/src/core/loops/sources/dataRowsMedia.ts b/src/core/loops/sources/dataRowsMedia.ts new file mode 100644 index 000000000..4f08813dc --- /dev/null +++ b/src/core/loops/sources/dataRowsMedia.ts @@ -0,0 +1,132 @@ +/** + * Media resolution for the `data.rows` loop source. + * + * Media ids live inside `cells_json`, not as SQL columns — the built-in + * `featuredMedia` cell plus every user-defined `media` field, including the + * ones nested inside repeater items. Resolving them in SQL would mean a join + * per field and a query shape that differs per table, so the ids are gathered + * in TypeScript instead: one pass over the page of rows collects every id, and + * a SINGLE batched `in (…)` query turns them into public paths. One round trip + * regardless of how many rows or how many media fields the slice touched. + * + * This lives beside `dataRows.ts` rather than inside it because the two answer + * different questions. That file decides WHICH rows a loop returns — the + * filter, the order, the page window. This one decides what a row's media + * cells CONTAIN once those rows are in hand. + */ + +import type { LoopSourceDb } from '@core/loops/types' +import { readFeaturedMediaCell, readMediaCellIds, readRepeaterCell } from '@core/data/cells' +import type { DataField, DataRowCells, RepeaterItemField } from '@core/data/schemas' + +interface MediaAssetRow { + id: string + public_path: string +} + +/** Media fields appear at the top level and inside repeater items alike. */ +type MediaProjectionField = DataField | RepeaterItemField + +/** Dialect-appropriate positional placeholder: `$1` on Postgres, `?` on SQLite. */ +function positionalParam(db: LoopSourceDb, index: number): string { + return db.dialect === 'postgres' ? `$${index}` : '?' +} + +/** + * Resolve a set of media asset ids to their `public_path` values in one query. + * Uses `db.unsafe` with dialect-appropriate positional placeholders so the same + * code works on both Postgres and SQLite. Ids absent from the database — or + * soft-deleted — are absent from the returned map. + */ +export async function resolveMediaIdsToPaths( + db: LoopSourceDb, + ids: Iterable, +): Promise> { + const idList = [...new Set(ids)] + const pathMap = new Map() + if (idList.length === 0) return pathMap + const placeholders = idList.map((_, i) => positionalParam(db, i + 1)).join(', ') + const { rows } = await db.unsafe( + `select id, public_path + from media_assets + where id in (${placeholders}) and deleted_at is null`, + idList, + ) + for (const row of rows) pathMap.set(row.id, row.public_path) + return pathMap +} + +function collectFieldMediaIds( + cells: DataRowCells, + fields: readonly MediaProjectionField[], + ids: string[], +): void { + for (const field of fields) { + if (field.type === 'media') { + ids.push(...readMediaCellIds(cells, field.id)) + continue + } + if (field.type !== 'repeater') continue + for (const item of readRepeaterCell(cells, field.id)) { + collectFieldMediaIds(item.cells, field.fields, ids) + } + } +} + +/** + * Collect every media id referenced by a page of rows: the built-in + * `featuredMedia` cell plus every schema-declared media field. Repeater media + * is traversed recursively, and multi-value cells contribute every id while + * still resolving through one batched query. + */ +export function collectMediaIds( + rows: Array<{ cells_json: Record }>, + fields: readonly DataField[], +): string[] { + const ids: string[] = [] + for (const row of rows) { + const cells = row.cells_json as DataRowCells + const featured = readFeaturedMediaCell(cells) + if (featured) ids.push(featured) + collectFieldMediaIds(cells, fields, ids) + } + return ids +} + +/** + * Resolve schema-declared media ids without changing collection cardinality: + * scalar media becomes a public path (or null), multi-media stays an ordered + * array of resolvable public paths, and repeater items keep their `{ id, cells }` + * shape while media inside `cells` is projected recursively. + */ +export function resolvedMediaOverlay( + cells: DataRowCells, + fields: readonly MediaProjectionField[], + mediaPathMap: Map, +): DataRowCells { + const overlay: DataRowCells = {} + for (const field of fields) { + if (field.type === 'media') { + const ids = readMediaCellIds(cells, field.id) + if (field.allowMultiple === true) { + overlay[field.id] = ids.flatMap((id) => { + const path = mediaPathMap.get(id) + return path ? [path] : [] + }) + } else { + const id = ids[0] + overlay[field.id] = id ? (mediaPathMap.get(id) ?? null) : null + } + continue + } + if (field.type !== 'repeater') continue + overlay[field.id] = readRepeaterCell(cells, field.id).map((item) => ({ + ...item, + cells: { + ...item.cells, + ...resolvedMediaOverlay(item.cells, field.fields, mediaPathMap), + }, + })) + } + return overlay +} From c1353f850c3895f59fe9cf4711f125a577123005 Mon Sep 17 00:00:00 2001 From: Mostafa Sadeghi <205455727+mostafasadeghidev@users.noreply.github.com> Date: Mon, 17 Aug 2026 18:39:09 +0200 Subject: [PATCH 5/5] feat(loops): pick a relation filter's value by name instead of typing an id MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A relation cell stores the referenced row's ID. With `cellValue` rendered as a free-text box, filtering "Category is Impact" meant the author had to type `z6W88XshljjzArfa4sV0f` — a string the editor never shows anywhere and gives them no way to look up. The control was present and unusable, which is worse than absent: it looks like the feature works. When the chosen field is a relation, the referenced table's rows now load and `cellValue` becomes a select of their names. Everything else keeps the text box, and the valueless operators still drop the control entirely. Rows are labelled by their `name` cell with the slug as fallback — the same identity the Data workspace shows in its own row list, so the option an author picks here reads the same as the row they know. The fetch follows the pattern already in this component: lazy, keyed on the resolved target table, and a failed load degrades to an empty list rather than blocking the panel. Co-Authored-By: Claude Opus 5 --- .../PropertiesPanel/LoopPropertiesView.tsx | 39 ++++++++++++++++++- 1 file changed, 37 insertions(+), 2 deletions(-) diff --git a/src/admin/pages/site/panels/PropertiesPanel/LoopPropertiesView.tsx b/src/admin/pages/site/panels/PropertiesPanel/LoopPropertiesView.tsx index a9d77c2a0..c2607cdbf 100644 --- a/src/admin/pages/site/panels/PropertiesPanel/LoopPropertiesView.tsx +++ b/src/admin/pages/site/panels/PropertiesPanel/LoopPropertiesView.tsx @@ -19,9 +19,9 @@ import { loopSourceRegistry } from '@core/loops/registry' import { ENTRY_FIELD_FILTER_KEY, ENTRY_FIELD_SOURCE_ID } from '@core/loops' import { CELL_ORDER_PREFIX } from '@core/loops/cellFilter' import type { LoopEntitySource } from '@core/loops/types' -import type { DataTableListItem } from '@core/data/schemas' +import type { DataRow, DataTableListItem } from '@core/data/schemas' import type { PropertyControl, PropertySchema } from '@core/module-engine' -import { listCmsDataTables } from '@core/persistence/cmsData' +import { listCmsDataRows, listCmsDataTables } from '@core/persistence/cmsData' import { getAncestors, type Page } from '@core/page-tree' import { PropertyControlRenderer } from '@site/property-controls/PropertyControlRenderer' import { @@ -60,6 +60,27 @@ export function LoopPropertiesView({ nodeId, props, activePage }: LoopProperties [sourceId], ) + // A relation cell stores the referenced row's ID, so a free-text value box + // would ask the author to type an opaque string the editor never shows them + // and they have no way to look up. When the chosen field is a relation, load + // the referenced table's rows so the value can be picked by name instead. + // Every other field type keeps the text box. + const relationTargetTableId = (() => { + if (sourceId !== 'data.rows' || typeof filters.cellField !== 'string') return null + const table = tables?.find((t) => t.id === filters.tableId) + const field = table?.fields.find((f) => f.id === filters.cellField) + return field?.type === 'relation' ? field.targetTableId : null + })() + + const { data: relationRows } = useAsyncResource( + () => ( + relationTargetTableId + ? listCmsDataRows(relationTargetTableId).catch(() => []) + : Promise.resolve(null) + ), + [relationTargetTableId], + ) + // Build the per-source filter schema with dynamic options patched in. function buildFilterSchema(): PropertySchema { if (!source) return {} @@ -97,6 +118,20 @@ export function LoopPropertiesView({ nodeId, props, activePage }: LoopProperties delete schema.cellValue } else if (valuelessOperator) { delete schema.cellValue + } else if (relationRows) { + // Rows are labelled by their `name` cell, falling back to the slug — + // the same identity the Data workspace shows in its own row list. + schema.cellValue = { + type: 'select', + label: 'Value', + options: [ + { label: '— Choose a row —', value: '' }, + ...relationRows.map((row) => ({ + label: typeof row.cells.name === 'string' && row.cells.name ? row.cells.name : row.slug, + value: row.id, + })), + ], + } } return schema }