From 29046e708ba517e6614a45e9508759a087faa34c Mon Sep 17 00:00:00 2001 From: Matt Warren Date: Tue, 28 Apr 2026 13:10:27 -0700 Subject: [PATCH 1/2] Improve result history entry labels to be more informative Co-authored-by: Copilot --- src/Client/features/historyManager.ts | 126 +++++++++++++++++-- src/Client/features/historyPanel.ts | 2 +- src/Client/tests/unit/historyManager.test.ts | 108 ++++++++++++++++ 3 files changed, 228 insertions(+), 8 deletions(-) diff --git a/src/Client/features/historyManager.ts b/src/Client/features/historyManager.ts index f525256..6cf83b2 100644 --- a/src/Client/features/historyManager.ts +++ b/src/Client/features/historyManager.ts @@ -26,8 +26,17 @@ export interface HistoryEntry { fileName: string; /** ISO 8601 timestamp of when the query was executed. */ timestamp: string; - /** First ~80 characters of the query text for display. */ + /** + * Short, human-friendly label for display. Derived from the first + * meaningful leading comment in the query when available, otherwise + * from the comment-stripped (server-minified) query text. + */ queryPreview: string; + /** + * Truncated server-minified (comment-stripped) query text, suitable for + * showing in a tooltip alongside `queryPreview`. + */ + minifiedPreview?: string; /** Cluster the query was run against. */ cluster?: string; /** Database the query was run against. */ @@ -38,6 +47,12 @@ export interface HistoryEntry { queryHash?: number; } +/** Maximum length of the display label stored in `queryPreview`. */ +const MAX_LABEL_LENGTH = 60; + +/** Maximum length of the minified query snippet stored in `minifiedPreview`. */ +const MAX_MINIFIED_PREVIEW_LENGTH = 200; + /** * Computes a 32-bit FNV-1a hash of a string. */ @@ -50,6 +65,79 @@ function fnv1aHash(str: string): number { return hash >>> 0; // ensure unsigned } +/** + * Strips leading/trailing decorative "border" characters and whitespace from a + * comment line (e.g. the `/`, `=`, `-`, `_`, `#`, `~` chars used to draw + * banners around comments). + */ +function stripCommentBorder(text: string): string { + return text + .replace(/^[\s/=\-_#~]+/, '') + .replace(/[\s/=\-_#~]+$/, ''); +} + +/** Returns true if the string contains at least one letter or digit. */ +function hasMeaningfulText(text: string): boolean { + return /[A-Za-z0-9]/.test(text); +} + +/** Collapses whitespace and trims to at most `max` characters. */ +function collapseAndTruncate(text: string, max: number): string { + const collapsed = text.replace(/\s+/g, ' ').trim(); + return collapsed.length > max ? collapsed.slice(0, max) : collapsed; +} + +/** + * Computes a human-friendly display label for a query result history entry. + * + * Strategy (Kusto only has `//` line comments): + * 1. Walk leading lines, skipping blanks and decorative-only comment lines. + * 2. Strip border characters (`/`, `=`, `-`, `_`, `#`, `~`) from comments. + * 3. Use the first comment line that contains real text (letters/digits). + * 4. Otherwise fall back to the minified query (no comments) if available, + * else the raw query text. + * + * Exported for unit testing. + */ +export function computeHistoryDisplayLabel(query: string | undefined, minifiedQuery?: string): string { + const fallback = 'query'; + if (!query || query.trim() === '') { + const minTrim = minifiedQuery?.trim(); + return minTrim ? collapseAndTruncate(minTrim, MAX_LABEL_LENGTH) : fallback; + } + + const lines = query.split(/\r?\n/); + + for (const raw of lines) { + const trimmed = raw.trim(); + if (trimmed === '') { + continue; + } + + // Single-line comment. + if (trimmed.startsWith('//')) { + const content = stripCommentBorder(trimmed.slice(2)); + if (hasMeaningfulText(content)) { + return collapseAndTruncate(content, MAX_LABEL_LENGTH); + } + continue; + } + + // First non-blank, non-comment line — leading comment block (if any) + // had no meaningful text. Stop walking and use the fallback below. + break; + } + + // Fall back to the comment-stripped server-minified query when available; + // otherwise use the original query text. + const minTrim = minifiedQuery?.trim(); + if (minTrim && minTrim.length > 0) { + return collapseAndTruncate(minTrim, MAX_LABEL_LENGTH); + } + const collapsed = collapseAndTruncate(query, MAX_LABEL_LENGTH); + return collapsed.length > 0 ? collapsed : fallback; +} + /** * Manages a persitent list of query history data */ @@ -90,10 +178,22 @@ export class HistoryManager { // ─── Public API ───────────────────────────────────────────────────── + /** + * Returns the server-minified (comment-stripped, whitespace-collapsed) + * form of `query`, or the original query if minification isn't available. + */ + private async getMinifiedQuery(query: string): Promise { + return (await this.server.getMinifiedQuery(query))?.minifiedQuery ?? query; + } + + /** Computes the canonical history hash for an already-minified query. */ + private hashMinifiedQuery(minifiedQuery: string): number { + return fnv1aHash(minifiedQuery); + } + /** Computes a hash of the server-minified query text. */ private async computeQueryHash(query: string): Promise { - const minified = (await this.server.getMinifiedQuery(query))?.minifiedQuery ?? query; - return fnv1aHash(minified); + return this.hashMinifiedQuery(await this.getMinifiedQuery(query)); } /** Returns all history entry metadata, most recent first. */ @@ -110,9 +210,17 @@ export class HistoryManager { const timestamp = now.toISOString(); const datePart = timestamp.replace(/[-:]/g, '').replace('T', '_').replace(/\.\d+Z$/, ''); - // Build a short label from the query text - const querySnippet = (resultData.query ?? 'query') - .replace(/\s+/g, ' ').trim().slice(0, 60); + // Fetch the server-minified (comment-stripped) query once so we can + // use it both to derive a meaningful display label and to compute the + // query hash. + const minifiedQuery = resultData.query + ? await this.getMinifiedQuery(resultData.query) + : undefined; + + // Build a short display label from the query text, preferring a + // meaningful comment when present and otherwise falling back to the + // minified query. + const querySnippet = computeHistoryDisplayLabel(resultData.query, minifiedQuery); // Sanitize for use as a filename const safeName = querySnippet.replace(/[<>:"/\\|?*\x00-\x1f]/g, '_').slice(0, 40); const fileName = `${datePart}_${safeName}.kqr`; @@ -122,12 +230,16 @@ export class HistoryManager { await fs.promises.writeFile(filePath, content, 'utf-8'); const rowCount = resultData.tables.reduce((sum, t) => sum + (t.rows?.length ?? 0), 0); - const queryHash = resultData.query ? await this.computeQueryHash(resultData.query) : undefined; + const queryHash = minifiedQuery !== undefined ? this.hashMinifiedQuery(minifiedQuery) : undefined; + const minifiedPreview = minifiedQuery !== undefined + ? collapseAndTruncate(minifiedQuery, MAX_MINIFIED_PREVIEW_LENGTH) + : undefined; const meta: HistoryEntry = { fileName, timestamp, queryPreview: querySnippet, + ...(minifiedPreview && { minifiedPreview }), ...(resultData.cluster !== undefined && { cluster: resultData.cluster }), ...(resultData.database !== undefined && { database: resultData.database }), rowCount, diff --git a/src/Client/features/historyPanel.ts b/src/Client/features/historyPanel.ts index 674d10a..3cc0491 100644 --- a/src/Client/features/historyPanel.ts +++ b/src/Client/features/historyPanel.ts @@ -131,7 +131,7 @@ class HistoryItem extends vscode.TreeItem { this.description = parts.join(' · '); this.tooltip = [ - meta.queryPreview, + meta.minifiedPreview ?? meta.queryPreview, `${meta.cluster ?? ''}${meta.database ? '/' + meta.database : ''}`, timeStr, ].filter(Boolean).join('\n'); diff --git a/src/Client/tests/unit/historyManager.test.ts b/src/Client/tests/unit/historyManager.test.ts index d8e2d49..f816f68 100644 --- a/src/Client/tests/unit/historyManager.test.ts +++ b/src/Client/tests/unit/historyManager.test.ts @@ -7,6 +7,7 @@ import * as os from 'os'; import * as path from 'path'; import { HistoryManager } from '../../features/historyManager'; import type { HistoryEntry } from '../../features/historyManager'; +import { computeHistoryDisplayLabel } from '../../features/historyManager'; import type { ResultData, IServer } from '../../features/server'; import { NullServer } from '../../features/server'; import type * as vscode from 'vscode'; @@ -358,3 +359,110 @@ describe('HistoryManager', () => { }); }); }); + +describe('computeHistoryDisplayLabel', () => { + it('returns "query" for empty input with no minified query', () => { + expect(computeHistoryDisplayLabel('')).toBe('query'); + expect(computeHistoryDisplayLabel(undefined)).toBe('query'); + }); + + it('uses the raw query when there are no comments', () => { + expect(computeHistoryDisplayLabel('StormEvents | take 10')) + .toBe('StormEvents | take 10'); + }); + + it('uses a meaningful single-line comment', () => { + const query = '// Find top error sources\nStormEvents | take 10'; + expect(computeHistoryDisplayLabel(query)).toBe('Find top error sources'); + }); + + it('skips decorative banner lines and uses the inner comment', () => { + const query = [ + '////////////////////////////////', + '// Find top error sources', + '////////////////////////////////', + 'StormEvents | take 10', + ].join('\n'); + expect(computeHistoryDisplayLabel(query)).toBe('Find top error sources'); + }); + + it('skips bare comment markers like "//"', () => { + const query = [ + '//', + '// Top error sources', + '//', + 'StormEvents', + ].join('\n'); + expect(computeHistoryDisplayLabel(query)).toBe('Top error sources'); + }); + + it('strips border characters from inside a comment line', () => { + const query = '//==== Top error sources ====\nStormEvents'; + expect(computeHistoryDisplayLabel(query)).toBe('Top error sources'); + }); + + it('falls back to the minified query when comments are pure decoration', () => { + const query = [ + '////////////////////////////////', + '//', + '////////////////////////////////', + '// the comment is just noise', + 'StormEvents | take 10', + ].join('\n'); + // Pretend the leading comment is "noise" only — simulate by passing a + // minified body and a query whose comments are all decorative. + const decorativeOnly = [ + '////////////////////////////////', + '////////////////////////////////', + 'StormEvents | take 10', + ].join('\n'); + expect(computeHistoryDisplayLabel(decorativeOnly, 'StormEvents | take 10')) + .toBe('StormEvents | take 10'); + // The first version finds a meaningful comment. + expect(computeHistoryDisplayLabel(query)).toBe('the comment is just noise'); + }); + + it('falls back to the raw query when no minified is provided and no useful comment', () => { + const query = '////\n////\nStormEvents | take 10'; + expect(computeHistoryDisplayLabel(query)).toBe('StormEvents | take 10'); + }); + + it('prefers minified query over raw when falling back', () => { + const query = '// hello world is something\nStormEvents\n | take 10'; + // Minified strips comments and collapses whitespace. + const minified = 'StormEvents | take 10'; + // Comment is meaningful, so should still use the comment. + expect(computeHistoryDisplayLabel(query, minified)).toBe('hello world is something'); + }); + + it('uses minified query when leading comment block is decorative-only', () => { + const query = [ + '//', + '// ====', + '//', + 'StormEvents | where State == "TX"', + ].join('\n'); + const minified = 'StormEvents|where State=="TX"'; + expect(computeHistoryDisplayLabel(query, minified)).toBe('StormEvents|where State=="TX"'); + }); + + it('truncates labels to 60 characters', () => { + const long = 'A'.repeat(200); + expect(computeHistoryDisplayLabel(long).length).toBeLessThanOrEqual(60); + }); + + it('collapses internal whitespace', () => { + expect(computeHistoryDisplayLabel('// spaced out title\nStormEvents')) + .toBe('spaced out title'); + }); + + it('handles CRLF line endings', () => { + const query = '// Title here\r\nStormEvents | take 10'; + expect(computeHistoryDisplayLabel(query)).toBe('Title here'); + }); + + it('skips blank lines before a comment', () => { + const query = '\n\n \n// real title\nStormEvents'; + expect(computeHistoryDisplayLabel(query)).toBe('real title'); + }); +}); From 91424e45eac6c09b0e7825ef677b6dcf421f7343 Mon Sep 17 00:00:00 2001 From: Matt Warren Date: Tue, 28 Apr 2026 13:31:42 -0700 Subject: [PATCH 2/2] Fix failing tests and PR feedback Co-authored-by: Copilot --- src/Client/features/historyManager.ts | 26 +++++--- src/Client/tests/unit/historyManager.test.ts | 67 +++++++++++++++++++- 2 files changed, 81 insertions(+), 12 deletions(-) diff --git a/src/Client/features/historyManager.ts b/src/Client/features/historyManager.ts index 6cf83b2..3efac91 100644 --- a/src/Client/features/historyManager.ts +++ b/src/Client/features/historyManager.ts @@ -76,15 +76,20 @@ function stripCommentBorder(text: string): string { .replace(/[\s/=\-_#~]+$/, ''); } -/** Returns true if the string contains at least one letter or digit. */ +/** Returns true if the string contains at least one letter or digit (any script). */ function hasMeaningfulText(text: string): boolean { - return /[A-Za-z0-9]/.test(text); + return /[\p{L}\p{N}]/u.test(text); } -/** Collapses whitespace and trims to at most `max` characters. */ +/** + * Collapses whitespace and truncates to at most `max` Unicode code points. + * Uses `Array.from` so surrogate pairs (e.g., emoji, supplementary-plane CJK) + * are kept intact rather than split mid-character. + */ function collapseAndTruncate(text: string, max: number): string { const collapsed = text.replace(/\s+/g, ' ').trim(); - return collapsed.length > max ? collapsed.slice(0, max) : collapsed; + const codePoints = Array.from(collapsed); + return codePoints.length > max ? codePoints.slice(0, max).join('') : collapsed; } /** @@ -95,7 +100,7 @@ function collapseAndTruncate(text: string, max: number): string { * 2. Strip border characters (`/`, `=`, `-`, `_`, `#`, `~`) from comments. * 3. Use the first comment line that contains real text (letters/digits). * 4. Otherwise fall back to the minified query (no comments) if available, - * else the raw query text. + * else the remaining query text past the leading comment block. * * Exported for unit testing. */ @@ -107,9 +112,10 @@ export function computeHistoryDisplayLabel(query: string | undefined, minifiedQu } const lines = query.split(/\r?\n/); + let firstCodeLine = lines.length; - for (const raw of lines) { - const trimmed = raw.trim(); + for (let i = 0; i < lines.length; i++) { + const trimmed = lines[i]!.trim(); if (trimmed === '') { continue; } @@ -125,16 +131,18 @@ export function computeHistoryDisplayLabel(query: string | undefined, minifiedQu // First non-blank, non-comment line — leading comment block (if any) // had no meaningful text. Stop walking and use the fallback below. + firstCodeLine = i; break; } // Fall back to the comment-stripped server-minified query when available; - // otherwise use the original query text. + // otherwise use the original query text past the leading comment block. const minTrim = minifiedQuery?.trim(); if (minTrim && minTrim.length > 0) { return collapseAndTruncate(minTrim, MAX_LABEL_LENGTH); } - const collapsed = collapseAndTruncate(query, MAX_LABEL_LENGTH); + const remainder = lines.slice(firstCodeLine).join('\n'); + const collapsed = collapseAndTruncate(remainder, MAX_LABEL_LENGTH); return collapsed.length > 0 ? collapsed : fallback; } diff --git a/src/Client/tests/unit/historyManager.test.ts b/src/Client/tests/unit/historyManager.test.ts index f816f68..8b15167 100644 --- a/src/Client/tests/unit/historyManager.test.ts +++ b/src/Client/tests/unit/historyManager.test.ts @@ -427,14 +427,26 @@ describe('computeHistoryDisplayLabel', () => { expect(computeHistoryDisplayLabel(query)).toBe('StormEvents | take 10'); }); - it('prefers minified query over raw when falling back', () => { + it('uses a meaningful comment even when a minified query is provided', () => { const query = '// hello world is something\nStormEvents\n | take 10'; - // Minified strips comments and collapses whitespace. const minified = 'StormEvents | take 10'; - // Comment is meaningful, so should still use the comment. + // Comment is meaningful, so it wins over both the minified and raw bodies. expect(computeHistoryDisplayLabel(query, minified)).toBe('hello world is something'); }); + it('prefers the minified query over the raw body when falling back', () => { + // Decorative-only comments → falls back. Minified differs from raw, + // so the choice between them is observable. + const query = [ + '////', + '////', + 'StormEvents', + ' | take 10', + ].join('\n'); + const minified = 'StormEvents|take 10'; + expect(computeHistoryDisplayLabel(query, minified)).toBe('StormEvents|take 10'); + }); + it('uses minified query when leading comment block is decorative-only', () => { const query = [ '//', @@ -465,4 +477,53 @@ describe('computeHistoryDisplayLabel', () => { const query = '\n\n \n// real title\nStormEvents'; expect(computeHistoryDisplayLabel(query)).toBe('real title'); }); + + it('recognizes non-ASCII letters as meaningful (accented Latin)', () => { + const query = '// Événements récents\nStormEvents'; + expect(computeHistoryDisplayLabel(query)).toBe('Événements récents'); + }); + + it('recognizes Cyrillic letters as meaningful', () => { + const query = '// Топ ошибки\nStormEvents'; + expect(computeHistoryDisplayLabel(query)).toBe('Топ ошибки'); + }); + + it('recognizes CJK characters as meaningful', () => { + const query = '// 上位エラー\nStormEvents'; + expect(computeHistoryDisplayLabel(query)).toBe('上位エラー'); + }); + + it('does not split surrogate pairs when truncating', () => { + // Each emoji is a surrogate pair (2 UTF-16 units, 1 code point). + // Use a mix of letters + many emojis so the comment is considered + // meaningful and the truncation crosses surrogate-pair boundaries. + const emoji = '😀'; + const query = '// title ' + emoji.repeat(100) + '\nStormEvents'; + const label = computeHistoryDisplayLabel(query); + // Label should have at most MAX_LABEL_LENGTH (60) code points, + // and the result must round-trip through Array.from with no lone + // surrogates (a split surrogate would survive as a length-1 string + // unit unequal to itself reconstituted). + const codePoints = Array.from(label); + expect(codePoints.length).toBeLessThanOrEqual(60); + // Reconstructing from code points should equal the label exactly. + // If a surrogate pair were split, the second-half lone surrogate + // would still appear, but Array.from(label).join('') would still + // produce the same string, so check directly that no UTF-16 unit + // is an unpaired surrogate. + for (let i = 0; i < label.length; i++) { + const code = label.charCodeAt(i); + const isHighSurrogate = code >= 0xD800 && code <= 0xDBFF; + const isLowSurrogate = code >= 0xDC00 && code <= 0xDFFF; + if (isHighSurrogate) { + // Must be followed by a low surrogate. + const next = label.charCodeAt(i + 1); + expect(next >= 0xDC00 && next <= 0xDFFF).toBe(true); + i++; // Skip the paired low surrogate. + } else { + // Must NOT be a lone low surrogate. + expect(isLowSurrogate).toBe(false); + } + } + }); });