From 5436548972c621ef4827db3221e5cfaa2e5ad1f9 Mon Sep 17 00:00:00 2001 From: Earl Plak Date: Sat, 27 Jun 2026 18:35:52 +0200 Subject: [PATCH 1/4] fix(dataview): dedicated list/metadata formatters so non-raw output renders (#220) dataview.list and dataview.metadata non-raw output always rendered "No results found": the dispatcher force-cast their {success,count,pages} and {success,path,metadata} producer shapes into the {values} query formatter (formatDataviewQuery reads response.values), which always hit the empty branch. raw:true was unaffected. A page list and a single page's metadata aren't query results, so add dedicated formatDataviewPages and formatDataviewMetadata and route the two dispatcher cases to them instead of formatDataviewQuery. Both surface producer-side failures (bad source, page-not-found) instead of a misleading empty result. Adds mock-faithful regression tests through formatResponse() covering both happy paths and failure paths (the #216 mock-faithfulness lesson). Verified live against Dataview 0.5.68. --- src/formatters/dataview.ts | 144 +++++++++++++++++++++++++++++ src/formatters/index.ts | 11 ++- tests/dataview-integration.test.ts | 63 +++++++++++++ 3 files changed, 217 insertions(+), 1 deletion(-) diff --git a/src/formatters/dataview.ts b/src/formatters/dataview.ts index 9c86fac..31fdf0b 100644 --- a/src/formatters/dataview.ts +++ b/src/formatters/dataview.ts @@ -192,6 +192,150 @@ export function formatDataviewStatus(response: DataviewStatusResponse): string { return joinLines(lines); } +/** + * Format dataview.list response + * + * `listPages()` returns `{ success, source, count, pages: [...] }`, not the + * `{ values }` shape `formatDataviewQuery` reads — routing it through the query + * formatter always hit the `!values` branch and rendered "No results found" + * regardless of data (#220). A page list isn't really a query result, so it + * gets its own formatter. + */ +export interface DataviewPagesResponse { + success: boolean; + source?: string; + count?: number; + pages?: Array>; + error?: string; +} + +export function formatDataviewPages(response: DataviewPagesResponse): string { + const lines: string[] = []; + + lines.push(header(1, 'Dataview: Pages')); + lines.push(''); + lines.push(property('Source', response.source ?? 'all', 0)); + + if (response.success === false) { + lines.push(''); + lines.push(`❌ Query failed: ${response.error || 'Unknown error'}`); + lines.push(summaryFooter()); + return joinLines(lines); + } + + const pages = response.pages ?? []; + const total = response.count ?? pages.length; + lines.push(property('Count', String(total), 0)); + lines.push(''); + + if (pages.length === 0) { + lines.push('No pages found.'); + lines.push(summaryFooter()); + return joinLines(lines); + } + + pages.slice(0, 30).forEach((page, i) => { + const path = typeof page.path === 'string' ? page.path : JSON.stringify(page); + lines.push(`${i + 1}. ${truncate(path, 60)}`); + }); + + if (total > 30) { + lines.push(`\n... and ${total - 30} more pages`); + } + + lines.push(divider()); + lines.push(tip('Use `dataview.metadata(path)` for one page, or `vault.read(path)` to open it')); + lines.push(summaryFooter()); + + return joinLines(lines); +} + +/** + * Format dataview.metadata response + * + * `getPageMetadata()` returns `{ success, path, metadata: {...} }` — same #220 + * shape mismatch as the page list. A single page's metadata is not a query + * result either, so it gets a dedicated formatter rather than the `{ values }` + * query path. + */ +export interface DataviewMetadataResponse { + success: boolean; + path: string; + metadata?: { + file?: Record; + tags?: unknown[]; + aliases?: unknown[]; + outlinks?: unknown[]; + inlinks?: unknown[]; + tasks?: number; + lists?: number; + custom?: Record; + }; + error?: string; +} + +export function formatDataviewMetadata(response: DataviewMetadataResponse): string { + const lines: string[] = []; + + lines.push(header(1, 'Dataview: Metadata')); + lines.push(''); + lines.push(property('Path', response.path, 0)); + + if (response.success === false || !response.metadata) { + lines.push(''); + lines.push(`❌ ${response.error || 'No metadata available'}`); + lines.push(summaryFooter()); + return joinLines(lines); + } + + const m = response.metadata; + const tags = Array.isArray(m.tags) ? m.tags : []; + const aliases = Array.isArray(m.aliases) ? m.aliases : []; + const outlinks = Array.isArray(m.outlinks) ? m.outlinks : []; + const inlinks = Array.isArray(m.inlinks) ? m.inlinks : []; + + lines.push(''); + if (tags.length > 0) { + lines.push(property('Tags', tags.map(t => String(t)).join(', '), 0)); + } + if (aliases.length > 0) { + lines.push(property('Aliases', aliases.map(a => String(a)).join(', '), 0)); + } + lines.push(property('Outlinks', String(outlinks.length), 0)); + lines.push(property('Inlinks', String(inlinks.length), 0)); + lines.push(property('Tasks', String(m.tasks ?? 0), 0)); + lines.push(property('Lists', String(m.lists ?? 0), 0)); + + const custom = (m.custom && typeof m.custom === 'object') ? m.custom : {}; + const customKeys = Object.keys(custom); + if (customKeys.length > 0) { + lines.push(''); + lines.push(header(2, 'Frontmatter')); + customKeys.slice(0, 15).forEach(key => { + const value = custom[key]; + let display: string; + if (value === null || value === undefined) { + display = String(value); + } else if (typeof value === 'object') { + display = JSON.stringify(value); + } else { + const primitive = value as string | number | boolean | bigint | symbol; + display = String(primitive); + } + lines.push(property(key, truncate(display, 50), 0)); + }); + if (customKeys.length > 15) { + lines.push(`... and ${customKeys.length - 15} more fields`); + } + } + + lines.push(divider()); + lines.push(tip('Use `vault.read(path)` to view the full note')); + lines.push(summaryFooter()); + + return joinLines(lines); +} + /** * Format bases.query response */ diff --git a/src/formatters/index.ts b/src/formatters/index.ts index 5781822..3894d54 100644 --- a/src/formatters/index.ts +++ b/src/formatters/index.ts @@ -67,6 +67,8 @@ import { import { formatDataviewQuery, formatDataviewStatus, + formatDataviewPages, + formatDataviewMetadata, formatBasesQuery, formatBasesList, formatBasesRead, @@ -74,6 +76,8 @@ import { formatBasesExport, DataviewQueryResponse, DataviewStatusResponse, + DataviewPagesResponse, + DataviewMetadataResponse, BasesQueryResponse, BasesListResponse, BasesReadResponse, @@ -167,6 +171,8 @@ export { // Dataview formatDataviewQuery, formatDataviewStatus, + formatDataviewPages, + formatDataviewMetadata, formatBasesQuery, formatBasesList, formatBasesRead, @@ -174,6 +180,8 @@ export { formatBasesExport, DataviewQueryResponse, DataviewStatusResponse, + DataviewPagesResponse, + DataviewMetadataResponse, BasesQueryResponse, BasesListResponse, BasesReadResponse, @@ -623,8 +631,9 @@ export function formatResponse( case 'dataview.status': return formatDataviewStatus(normalized as DataviewStatusResponse); case 'dataview.list': + return formatDataviewPages(normalized as DataviewPagesResponse); case 'dataview.metadata': - return formatDataviewQuery({ ...(normalized as Record), type: 'list', successful: true } as DataviewQueryResponse); + return formatDataviewMetadata(normalized as DataviewMetadataResponse); // Bases operations case 'bases.list': diff --git a/tests/dataview-integration.test.ts b/tests/dataview-integration.test.ts index 6ec70d5..f92c955 100644 --- a/tests/dataview-integration.test.ts +++ b/tests/dataview-integration.test.ts @@ -423,6 +423,69 @@ describe('Dataview Integration', () => { }); }); + // #220: dataview.list / dataview.metadata non-raw output always rendered + // "No results found" because their {count,pages} / {metadata} producer shapes + // were force-cast into the {values} query formatter. These exercise the full + // producer → formatResponse() path with the real Dataview producer shapes. + describe('Formatted output (#220)', () => { + test('dataview.list renders page paths and count, not "No results found"', () => { + const app = new MockApp(true, true); + const api = new MockObsidianAPI(app); + const tool = new DataviewTool(api as any); + + const output = formatResponse('dataview', 'list', tool.listPages('#tag'), false); + + expect(output).toContain('Dataview: Pages'); + expect(output).not.toContain('No results found'); + expect(output).toContain('Count'); + expect(output).toContain('Note1.md'); + expect(output).toContain('Note2.md'); + }); + + test('dataview.metadata renders page metadata and frontmatter, not "No results found"', () => { + const app = new MockApp(true, true); + const api = new MockObsidianAPI(app); + const tool = new DataviewTool(api as any); + + const output = formatResponse('dataview', 'metadata', tool.getPageMetadata('Note1.md'), false); + + expect(output).toContain('Dataview: Metadata'); + expect(output).not.toContain('No results found'); + expect(output).toContain('Note1.md'); + // custom frontmatter fields surfaced + expect(output).toContain('priority'); + expect(output).toContain('high'); + }); + + test('dataview.list surfaces a producer failure instead of a false "No results found"', () => { + const app = new MockApp(true, true); + const api = new MockObsidianAPI(app); + const tool = new DataviewTool(api as any); + + const dvApi = (app.plugins.plugins['dataview'] as any).api as MockDataviewAPI; + dvApi.pages = (() => { throw new Error('bad source'); }) as any; + + const output = formatResponse('dataview', 'list', tool.listPages('nonsense'), false); + + expect(output).toContain('Query failed'); + expect(output).toContain('bad source'); + expect(output).not.toContain('No results found'); + }); + + test('dataview.metadata surfaces a page-not-found failure', () => { + const app = new MockApp(true, true); + const api = new MockObsidianAPI(app); + const tool = new DataviewTool(api as any); + + // page() returns null for an unknown path → producer returns {success:false, error} + const output = formatResponse('dataview', 'metadata', tool.getPageMetadata('Missing.md'), false); + + expect(output).toContain('Dataview: Metadata'); + expect(output).toContain('Missing.md'); + expect(output).toContain('Page not found'); + }); + }); + describe('Tool Availability Detection', () => { test('should detect when Dataview tool is not available', () => { const app = new MockApp(false); From 1c4183116ac18a6fcea63e0400d1015ebb5b4503 Mon Sep 17 00:00:00 2001 From: Earl Plak Date: Sat, 27 Jun 2026 19:51:23 +0200 Subject: [PATCH 2/4] fix(dataview): render GROUP BY results (list + task) instead of leaking group wrappers (#220) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit GROUP BY nests rows under a group wrapper — {key, rows}, or a list-pair {$widget:'dataview:list-pair', key, value} for LIST. The producer's list branch passed wrappers through (the formatter then rendered them raw) and the task branch mangled each group into an empty task, so grouped rows never showed. formatQueryResult now unwraps groups (flattening inner DataArrays to plain arrays) in the list and task branches, and the list/task formatters render each group's key and rows. Adds DataArray-wrapped GROUP BY fixtures + tests through executeQuery -> formatResponse (mock-faithful). LIST GROUP BY verified live on Dataview 0.5.68; TASK GROUP BY covered by synthetic fixtures (no grouped-task data in the test vault). TABLE GROUP BY is outside #220's stated scope. --- src/formatters/dataview.ts | 126 ++++++++++++++++++++++++----- src/tools/dataview-tool.ts | 51 +++++++++--- tests/dataview-integration.test.ts | 81 +++++++++++++++++++ 3 files changed, 227 insertions(+), 31 deletions(-) diff --git a/src/formatters/dataview.ts b/src/formatters/dataview.ts index 31fdf0b..756eb7b 100644 --- a/src/formatters/dataview.ts +++ b/src/formatters/dataview.ts @@ -108,19 +108,80 @@ function formatDataviewTable(headers: string[], rows: DataviewValue[]): string { return lines.join('\n'); } +/** A GROUP BY result element. Dataview wraps each group as a list-pair + * (`{ $widget: 'dataview:list-pair', key, value }` for LIST) or `{ key, rows }` + * for table-style groups. Without unwrapping, the row formatters rendered the + * wrapper object itself instead of its members (#220). */ +interface DataviewGroup { + key: unknown; + rows: DataviewValue[]; +} + +function asGroup(item: DataviewValue): DataviewGroup | null { + if (item === null || typeof item !== 'object' || Array.isArray(item)) { + return null; + } + const obj = item; + const rows = obj.rows ?? obj.value; + if (Array.isArray(rows) && ('key' in obj || obj.$widget === 'dataview:list-pair')) { + return { key: obj.key, rows: rows as DataviewValue[] }; + } + return null; +} + +function groupKeyLabel(key: unknown): string { + if (key === null || key === undefined || key === '') { + return '(no group)'; + } + if (typeof key === 'string') { + return key; + } + if (typeof key === 'number' || typeof key === 'bigint' || typeof key === 'boolean') { + return String(key); + } + if (typeof key === 'object') { + const obj = key as Record; + const filePath = obj.path ?? (obj.file as Record | undefined)?.path; + return typeof filePath === 'string' ? filePath : JSON.stringify(key); + } + return JSON.stringify(key); +} + +function renderListItem(item: DataviewValue): string { + if (item && typeof item === 'object') { + const obj = item as Record; + const filePath = obj.path ?? (obj.file as Record | undefined)?.path; + return typeof filePath === 'string' ? filePath : JSON.stringify(item); + } + return String(item); +} + function formatDataviewList(items: DataviewValue[]): string { + // GROUP BY: every element is a list-pair group — render each group's key and + // its rows, not the wrapper object (#220). + const groups = items.map(asGroup); + if (items.length > 0 && groups.every(g => g !== null)) { + const lines: string[] = []; + groups.slice(0, 20).forEach(group => { + lines.push(`**${truncate(groupKeyLabel(group.key), 60)}** (${group.rows.length})`); + group.rows.slice(0, 30).forEach(row => { + lines.push(`- ${truncate(renderListItem(row), 60)}`); + }); + if (group.rows.length > 30) { + lines.push(` ... and ${group.rows.length - 30} more`); + } + lines.push(''); + }); + if (groups.length > 20) { + lines.push(`... and ${groups.length - 20} more groups`); + } + return lines.join('\n').trimEnd(); + } + const lines: string[] = []; items.slice(0, 30).forEach((item, i) => { - let text: string; - if (item && typeof item === 'object') { - const obj = item as Record; - const filePath = obj.path ?? (obj.file as Record | undefined)?.path; - text = typeof filePath === 'string' ? filePath : JSON.stringify(item); - } else { - text = String(item); - } - lines.push(`${i + 1}. ${truncate(text, 60)}`); + lines.push(`${i + 1}. ${truncate(renderListItem(item), 60)}`); }); if (items.length > 30) { @@ -137,22 +198,45 @@ interface DataviewTask { path?: string; } +function renderTask(taskItem: DataviewValue): string { + const task = (taskItem !== null && typeof taskItem === 'object' && !Array.isArray(taskItem) + ? taskItem + : {}) as DataviewTask; + const checkbox = task.completed ? '[x]' : '[ ]'; + const taskString = taskItem !== null && typeof taskItem === 'object' + ? JSON.stringify(taskItem) + : String(taskItem); + const text = task.text || task.task || taskString; + let line = `- ${checkbox} ${truncate(text, 60)}`; + if (task.path) { + line += `\n from: ${task.path}`; + } + return line; +} + function formatDataviewTasks(tasks: DataviewValue[]): string { + // GROUP BY: every element is a group — render its key then its tasks (#220). + const groups = tasks.map(asGroup); + if (tasks.length > 0 && groups.every(g => g !== null)) { + const lines: string[] = []; + groups.slice(0, 20).forEach(group => { + lines.push(`**${truncate(groupKeyLabel(group.key), 60)}** (${group.rows.length})`); + group.rows.slice(0, 30).forEach(taskItem => lines.push(renderTask(taskItem))); + if (group.rows.length > 30) { + lines.push(` ... and ${group.rows.length - 30} more`); + } + lines.push(''); + }); + if (groups.length > 20) { + lines.push(`... and ${groups.length - 20} more groups`); + } + return lines.join('\n').trimEnd(); + } + const lines: string[] = []; tasks.slice(0, 30).forEach(taskItem => { - const task = (taskItem !== null && typeof taskItem === 'object' && !Array.isArray(taskItem) - ? taskItem - : {}) as DataviewTask; - const checkbox = task.completed ? '[x]' : '[ ]'; - const taskString = taskItem !== null && typeof taskItem === 'object' - ? JSON.stringify(taskItem) - : String(taskItem); - const text = task.text || task.task || taskString; - lines.push(`- ${checkbox} ${truncate(text, 60)}`); - if (task.path) { - lines.push(` from: ${task.path}`); - } + lines.push(renderTask(taskItem)); }); if (tasks.length > 30) { diff --git a/src/tools/dataview-tool.ts b/src/tools/dataview-tool.ts index 1190152..1284ad5 100644 --- a/src/tools/dataview-tool.ts +++ b/src/tools/dataview-tool.ts @@ -58,6 +58,25 @@ function toPlainArray(value: unknown): unknown[] { return []; } +/** + * GROUP BY results nest rows under a group wrapper — `{ key, rows }`, or a + * list-pair `{ $widget: 'dataview:list-pair', key, value }` for LIST. The inner + * collection may itself be a Dataview DataArray, so it's run through + * toPlainArray. Returns null when the element is a plain row, not a group. + */ +function unwrapGroup(el: unknown): { key: unknown; rows: unknown[] } | null { + if (!el || typeof el !== 'object' || Array.isArray(el)) return null; + const obj = el as Record; + const inner = obj.rows ?? obj.value; + const isGroup = 'key' in obj || obj.$widget === 'dataview:list-pair'; + if (!isGroup) return null; + if (!Array.isArray(inner) && !(inner && typeof (inner as { array?: () => unknown[] }).array === 'function')) { + return null; + } + return { key: obj.key, rows: toPlainArray(inner) }; +} + + /** Dataview link value */ interface DataviewLink { path: string; @@ -373,9 +392,14 @@ export class DataviewTool { // Handle different result types switch (payload.type) { case 'list': + // GROUP BY: keep each group as {key, rows} (rows flattened to a plain + // array) instead of leaking the list-pair wrapper downstream (#220). return { type: 'list', - values: toPlainArray(payload.values) + values: toPlainArray(payload.values).map((el: unknown) => { + const group = unwrapGroup(el); + return group ? { key: group.key, rows: group.rows } : el; + }) }; case 'table': return { @@ -386,19 +410,26 @@ export class DataviewTool { return typeof tableRow?.array === 'function' ? tableRow.array() : row; }) }; - case 'task': + case 'task': { + const mapTask = (task: unknown) => { + const dvTask = task as DataviewTask; + return { + text: dvTask.text, + completed: dvTask.completed, + line: dvTask.line, + path: dvTask.path + }; + }; + // GROUP BY: preserve the group wrapper and map its inner rows as tasks, + // rather than mangling each group into an empty task (#220). return { type: 'task', - values: toPlainArray(payload.values).map((task: unknown) => { - const dvTask = task as DataviewTask; - return { - text: dvTask.text, - completed: dvTask.completed, - line: dvTask.line, - path: dvTask.path - }; + values: toPlainArray(payload.values).map((el: unknown) => { + const group = unwrapGroup(el); + return group ? { key: group.key, rows: group.rows.map(mapTask) } : mapTask(el); }) }; + } case 'calendar': return { type: 'calendar', diff --git a/tests/dataview-integration.test.ts b/tests/dataview-integration.test.ts index f92c955..08cc3ab 100644 --- a/tests/dataview-integration.test.ts +++ b/tests/dataview-integration.test.ts @@ -41,6 +41,45 @@ class MockDataviewAPI { // monad — { successful, value, error } — where value.values is a PLAIN // array (not a wrapped DataArray). The previous mock used the wrong flat + // wrapped shape, which is why CI stayed green while real Dataview broke (#216). + + // GROUP BY: Dataview nests rows under group wrappers, and the collections + // are DataArrays (`.array()`), not plain arrays. Mocked faithfully so the + // producer's group-unwrap + flatten is actually exercised (#220). + if (query.includes('GROUP BY') && query.includes('TASK')) { + return { + successful: true, + value: { + type: 'task', + values: { + array: () => [ + { key: 'open', rows: { array: () => [ + { text: 'task one', completed: false, path: 'Note1.md' }, + { text: 'task two', completed: true, path: 'Note2.md' } + ] } }, + { key: null, rows: { array: () => [ + { text: 'ungrouped task', completed: false, path: 'Note3.md' } + ] } } + ] + } + } + }; + } + + if (query.includes('GROUP BY')) { + return { + successful: true, + value: { + type: 'list', + values: { + array: () => [ + { $widget: 'dataview:list-pair', key: 'groupA', value: { array: () => ['Note 1', 'Note 2'] } }, + { $widget: 'dataview:list-pair', key: null, value: { array: () => ['Note 3'] } } + ] + } + } + }; + } + if (query.includes('LIST')) { return { successful: true, @@ -486,6 +525,48 @@ describe('Dataview Integration', () => { }); }); + // #220 (folded in): GROUP BY queries nest rows under group wrappers; the + // list/task branches rendered the wrappers (or, for tasks, mangled them into + // empty tasks) instead of the grouped rows. Driven end-to-end through + // executeQuery → formatResponse with DataArray-wrapped fixtures. + describe('GROUP BY formatted output (#220)', () => { + test('LIST ... GROUP BY renders group keys and rows, not the list-pair wrapper', async () => { + const app = new MockApp(true, true); + const api = new MockObsidianAPI(app); + const tool = new DataviewTool(api as any); + + const result = await tool.executeQuery('LIST rows.file.name FROM #tag GROUP BY group'); + const output = formatResponse('dataview', 'query', result, false); + + expect(output).toContain('Dataview: LIST'); + expect(output).not.toContain('No results found'); + expect(output).not.toContain('list-pair'); // wrapper not leaked + expect(output).not.toContain('$widget'); + expect(output).toContain('groupA'); + expect(output).toContain('Note 1'); + expect(output).toContain('Note 2'); + expect(output).toContain('(no group)'); // null group key + expect(output).toContain('Note 3'); + }); + + test('TASK ... GROUP BY renders group keys and their tasks', async () => { + const app = new MockApp(true, true); + const api = new MockObsidianAPI(app); + const tool = new DataviewTool(api as any); + + const result = await tool.executeQuery('TASK FROM #tag GROUP BY status'); + const output = formatResponse('dataview', 'query', result, false); + + expect(output).toContain('Dataview: TASK'); + expect(output).not.toContain('No results found'); + expect(output).toContain('open'); // group key + expect(output).toContain('task one'); + expect(output).toContain('[x] task two'); // completed checkbox preserved + expect(output).toContain('(no group)'); + expect(output).toContain('ungrouped task'); + }); + }); + describe('Tool Availability Detection', () => { test('should detect when Dataview tool is not available', () => { const app = new MockApp(false); From 3cb2ad1f9088645db80a8581371d47109a143d5a Mon Sep 17 00:00:00 2001 From: Earl Plak Date: Sat, 27 Jun 2026 20:42:29 +0200 Subject: [PATCH 3/4] fix(dataview): coerce TABLE Link/date cells instead of dumping raw JSON (#220) Per the maintainer's #220 comment: TABLE cells that are Dataview Link objects rendered as raw JSON ({"path":...}) and Luxon DateTime values as quoted ISO strings. Adds a coerceCell helper (Link -> display/path, DateTime -> toISO(), native Date -> toISOString(), primitives as-is) and routes formatDataviewTable cells through it. Adds a TABLE regression test with a Link cell and a DateTime cell (DataArray-wrapped row). --- src/formatters/dataview.ts | 45 +++++++++++++++++++++++------- tests/dataview-integration.test.ts | 44 +++++++++++++++++++++++++++++ 2 files changed, 79 insertions(+), 10 deletions(-) diff --git a/src/formatters/dataview.ts b/src/formatters/dataview.ts index 756eb7b..f56ed63 100644 --- a/src/formatters/dataview.ts +++ b/src/formatters/dataview.ts @@ -67,6 +67,40 @@ export function formatDataviewQuery(response: DataviewQueryResponse): string { return joinLines(lines); } +/** + * Render a single table cell value. Dataview hands back rich objects — `Link` + * (`{ path, display }`) and Luxon `DateTime` (`toISO()`) — not primitives. + * Without coercion they dumped as raw JSON (`{"path":…}`) and quoted ISO + * strings (#220). Collapse a Link to its display/path, a DateTime to ISO, and + * leave primitives as-is. + */ +function coerceCell(val: unknown): string { + if (val === null || val === undefined) { + return ''; + } + if (typeof val === 'string') { + return val; + } + if (typeof val === 'number' || typeof val === 'boolean' || typeof val === 'bigint') { + return String(val); + } + if (typeof val === 'object') { + const obj = val as Record; + if (typeof obj.path === 'string') { + return typeof obj.display === 'string' && obj.display ? obj.display : obj.path; + } + if (typeof obj.toISO === 'function') { + const iso = (obj.toISO as () => string | null)(); + if (iso) return iso; + } + if (val instanceof Date) { + return val.toISOString(); + } + return JSON.stringify(val); + } + return JSON.stringify(val); +} + function formatDataviewTable(headers: string[], rows: DataviewValue[]): string { const lines: string[] = []; @@ -87,16 +121,7 @@ function formatDataviewTable(headers: string[], rows: DataviewValue[]): string { } else if (row !== null && typeof row === 'object') { val = row[headers[i]]; } - let display: string; - if (val === null || val === undefined) { - display = ''; - } else if (typeof val === 'object') { - display = JSON.stringify(val); - } else { - const primitive = val as string | number | boolean | bigint | symbol; - display = String(primitive); - } - return truncate(display, 30); + return truncate(coerceCell(val), 30); }); lines.push('| ' + cells.join(' | ') + (hasMore ? ' | ...' : '') + ' |'); }); diff --git a/tests/dataview-integration.test.ts b/tests/dataview-integration.test.ts index 08cc3ab..e2ae36c 100644 --- a/tests/dataview-integration.test.ts +++ b/tests/dataview-integration.test.ts @@ -90,6 +90,28 @@ class MockDataviewAPI { }; } + if (query.includes('TABLE') && query.includes('mtime')) { + // Rich TABLE: rows are DataArrays of cells; cells are a Link object and a + // Luxon-style DateTime (toISO), not primitives (#220 cell rendering). + return { + successful: true, + value: { + type: 'table', + headers: ['File', 'file.mtime'], + values: [ + { array: () => [ + { path: 'Note1.md', display: 'Note1', type: 'file' }, + { toISO: () => '2026-01-02T03:04:05.000Z' } + ] }, + { array: () => [ + { path: 'Note2.md' }, + { toISO: () => '2026-01-03T00:00:00.000Z' } + ] } + ] + } + }; + } + if (query.includes('TABLE')) { return { successful: true, @@ -567,6 +589,28 @@ describe('Dataview Integration', () => { }); }); + // #220 (folded in, from the maintainer's comment): TABLE cells that are + // Dataview Link / DateTime objects rendered as raw JSON / quoted ISO strings. + describe('TABLE cell rendering (#220)', () => { + test('Link cells collapse to display/path and dates render unquoted', async () => { + const app = new MockApp(true, true); + const api = new MockObsidianAPI(app); + const tool = new DataviewTool(api as any); + + const result = await tool.executeQuery('TABLE file.mtime FROM ""'); + const output = formatResponse('dataview', 'query', result, false); + + expect(output).toContain('Dataview: TABLE'); + // Link → display (first row) / path (second row), not raw JSON + expect(output).not.toContain('{"path"'); + expect(output).toContain('Note1'); // display + expect(output).toContain('Note2.md'); // path fallback + // DateTime → bare ISO, no surrounding quotes + expect(output).toContain('2026-01-02T03:04:05.000Z'); + expect(output).not.toContain('"2026-01-02T03:04:05.000Z"'); + }); + }); + describe('Tool Availability Detection', () => { test('should detect when Dataview tool is not available', () => { const app = new MockApp(false); From b9a05e5aa13420ff5ed45cfa52a92ea47e88eb5e Mon Sep 17 00:00:00 2001 From: Aaron Bockelie Date: Fri, 3 Jul 2026 20:59:19 -0500 Subject: [PATCH 4/4] fix(dataview): harden GROUP BY group detection from maintainer review (#220) Maintainer review-fix commit on top of the contributor's work in PR #249, addressing three low-severity robustness findings from a security/quality review of the formatter changes: - F1: the group branches switched all-or-nothing on `groups.every(...)`, so a single unrecognized group in a GROUP BY result dropped every group back to the flat renderer (re-surfacing the #220 raw-wrapper symptom). Render grouped when *any* element is a group and emit a stray non-group element as a plain row, so one bad group can't collapse the whole result. - F2: `asGroup` (formatter) accepted only plain-array inner collections while the producer's `unwrapGroup` also accepted `.array()`-able DataArrays. They reconcile today only because the producer flattens first; align `asGroup`'s predicate and flatten so an unflattened wrapper reaching the formatter still renders instead of leaking. - F5: guard `toISO()`'s return with a `typeof === 'string'` check so a non-string return can't propagate into `truncate`. Happy-path GROUP BY behavior unchanged; all 352 tests pass. The mixed-group and unflattened-group paths are defensive (the current producer can't emit those shapes), so they are hardening rather than newly-reachable behavior. --- src/formatters/dataview.ts | 79 +++++++++++++++++++++++++------------- 1 file changed, 53 insertions(+), 26 deletions(-) diff --git a/src/formatters/dataview.ts b/src/formatters/dataview.ts index f56ed63..23eaf9d 100644 --- a/src/formatters/dataview.ts +++ b/src/formatters/dataview.ts @@ -90,8 +90,8 @@ function coerceCell(val: unknown): string { return typeof obj.display === 'string' && obj.display ? obj.display : obj.path; } if (typeof obj.toISO === 'function') { - const iso = (obj.toISO as () => string | null)(); - if (iso) return iso; + const iso = (obj.toISO as () => unknown)(); + if (typeof iso === 'string' && iso) return iso; } if (val instanceof Date) { return val.toISOString(); @@ -147,9 +147,20 @@ function asGroup(item: DataviewValue): DataviewGroup | null { return null; } const obj = item; - const rows = obj.rows ?? obj.value; - if (Array.isArray(rows) && ('key' in obj || obj.$widget === 'dataview:list-pair')) { - return { key: obj.key, rows: rows as DataviewValue[] }; + if (!('key' in obj || obj.$widget === 'dataview:list-pair')) { + return null; + } + const inner = obj.rows ?? obj.value; + // The producer (`unwrapGroup`) normally flattens the inner DataArray to a + // plain array before a group reaches here. Accept a raw DataArray (`.array()`) + // too, so the formatter stays in agreement with the producer's group + // predicate rather than silently leaking a wrapper if an unflattened group + // ever reaches it (PR #249 review, F2). + if (Array.isArray(inner)) { + return { key: obj.key, rows: inner as DataviewValue[] }; + } + if (inner && typeof (inner as { array?: () => unknown[] }).array === 'function') { + return { key: obj.key, rows: (inner as { array: () => DataviewValue[] }).array() }; } return null; } @@ -182,23 +193,31 @@ function renderListItem(item: DataviewValue): string { } function formatDataviewList(items: DataviewValue[]): string { - // GROUP BY: every element is a list-pair group — render each group's key and - // its rows, not the wrapper object (#220). + // GROUP BY: elements arrive as list-pair group wrappers — render each group's + // key and its rows, not the wrapper object (#220). Switch to grouped + // rendering if *any* element is a group (not all), and render a stray + // non-group element as a plain top-level row, so one unrecognized group can't + // collapse the whole result back to raw wrappers (PR #249 review, F1). const groups = items.map(asGroup); - if (items.length > 0 && groups.every(g => g !== null)) { + if (items.length > 0 && groups.some(g => g !== null)) { const lines: string[] = []; - groups.slice(0, 20).forEach(group => { - lines.push(`**${truncate(groupKeyLabel(group.key), 60)}** (${group.rows.length})`); - group.rows.slice(0, 30).forEach(row => { - lines.push(`- ${truncate(renderListItem(row), 60)}`); - }); - if (group.rows.length > 30) { - lines.push(` ... and ${group.rows.length - 30} more`); + items.slice(0, 20).forEach((item, i) => { + const group = groups[i]; + if (group) { + lines.push(`**${truncate(groupKeyLabel(group.key), 60)}** (${group.rows.length})`); + group.rows.slice(0, 30).forEach(row => { + lines.push(`- ${truncate(renderListItem(row), 60)}`); + }); + if (group.rows.length > 30) { + lines.push(` ... and ${group.rows.length - 30} more`); + } + } else { + lines.push(`- ${truncate(renderListItem(item), 60)}`); } lines.push(''); }); - if (groups.length > 20) { - lines.push(`... and ${groups.length - 20} more groups`); + if (items.length > 20) { + lines.push(`... and ${items.length - 20} more groups`); } return lines.join('\n').trimEnd(); } @@ -240,20 +259,28 @@ function renderTask(taskItem: DataviewValue): string { } function formatDataviewTasks(tasks: DataviewValue[]): string { - // GROUP BY: every element is a group — render its key then its tasks (#220). + // GROUP BY: elements arrive as group wrappers — render each group's key then + // its tasks (#220). Switch to grouped rendering if *any* element is a group + // (not all), and render a stray non-group element as a plain task, so one + // unrecognized group can't collapse the whole result (PR #249 review, F1). const groups = tasks.map(asGroup); - if (tasks.length > 0 && groups.every(g => g !== null)) { + if (tasks.length > 0 && groups.some(g => g !== null)) { const lines: string[] = []; - groups.slice(0, 20).forEach(group => { - lines.push(`**${truncate(groupKeyLabel(group.key), 60)}** (${group.rows.length})`); - group.rows.slice(0, 30).forEach(taskItem => lines.push(renderTask(taskItem))); - if (group.rows.length > 30) { - lines.push(` ... and ${group.rows.length - 30} more`); + tasks.slice(0, 20).forEach((taskItem, i) => { + const group = groups[i]; + if (group) { + lines.push(`**${truncate(groupKeyLabel(group.key), 60)}** (${group.rows.length})`); + group.rows.slice(0, 30).forEach(row => lines.push(renderTask(row))); + if (group.rows.length > 30) { + lines.push(` ... and ${group.rows.length - 30} more`); + } + } else { + lines.push(renderTask(taskItem)); } lines.push(''); }); - if (groups.length > 20) { - lines.push(`... and ${groups.length - 20} more groups`); + if (tasks.length > 20) { + lines.push(`... and ${tasks.length - 20} more groups`); } return lines.join('\n').trimEnd(); }