mirror of
https://github.com/olehomelchenko/astrolabe.git
synced 2026-08-08 02:02:33 +00:00
Chart builder: drop unparseable expressions mid-edit, polite glyphed inline feedback
This commit is contained in:
@@ -368,6 +368,19 @@ bullet is the contract; cite it, not the source.)_
|
||||
other shows the text visually with no live role. Two live regions would announce the same
|
||||
message twice.
|
||||
|
||||
**Resolved — inline _live_ validation feedback is polite, glyphed, and field-linked.** A
|
||||
validator that re-checks on **every keystroke** (the Chart Builder expression inputs — a
|
||||
calculated field, a filter in expression mode) is **not** an assertive `alert`: that would
|
||||
interrupt on each character (APG _Alert_ "avoid frequent interruptions"; WCAG 2.2.4). It is a
|
||||
**polite `role="status"`** line, and severity reads from a **status glyph** (round error /
|
||||
triangle warning, arch 09 §5.2) **plus** colour — never colour alone (§3; WCAG 1.4.1), exactly
|
||||
like the guidance warnings above it. The owning `<input>` carries `aria-invalid` for the state
|
||||
and `aria-describedby` pointing at the message node so the text is available on focus, not only
|
||||
when it changes (GOV.UK _error-message_ field association). Contrast the editor's **render**
|
||||
error, which is a discrete, post-debounce result and stays the single `alert` of the rule
|
||||
above. _(Consulted via /council → WAI-ARIA APG Alert, GOV.UK error-message, NN/g #9. This
|
||||
bullet is the contract; cite it, not the source.)_
|
||||
|
||||
**Resolved — feature-modal dismissal & initial focus.** A feature modal (Datasets, Chart
|
||||
Builder) is a **passive** `dialog-modal`: dismissed by the close
|
||||
button, Escape, or a backdrop click (a passive modal carries no in-flight transaction, so
|
||||
|
||||
@@ -25,7 +25,34 @@
|
||||
Newest first. The at-a-glance build-order tracker is §4; per-item detail is §3. This log is
|
||||
the quick "where are we" — read it first.
|
||||
|
||||
- **2026-06-11** — **1C + 1D + 1E shipped (the Data section). Up next: Phase 2 (2A value-or-field channels, then 2B field shelf).**
|
||||
- **2026-06-11 (later)** — **Owed debts on the Data section closed. Up next: Phase 2 (2A value-or-field channels, then 2B field shelf).**
|
||||
- **Council pass** on the new error/disclosure copy (the previously-deferred auto-fire
|
||||
surface). Three a11y conformance gaps against `architecture/10` were fixed: the inline
|
||||
expression feedback now carries a **status glyph** (round error / triangle warning), not
|
||||
colour alone (§3, WCAG 1.4.1); the parse error is a **polite `role="status"`**, not a
|
||||
per-keystroke assertive alert (APG Alert / WCAG 2.2.4); and the message is linked to its
|
||||
input via **`aria-describedby`** (GOV.UK error-message). Unknown-field copy clarified to
|
||||
"…— not a column in this dataset." Resolution recorded in `architecture/10` §5.
|
||||
- **Manual/visual pass** run via a headless-Chrome (Playwright) walk-through of the live
|
||||
builder — filter shelf (predicate + `is between`), field↔expression toggle, expression
|
||||
error/unknown-field glyphs (filter _and_ calc inputs), calculated field, data-preview
|
||||
table with type chips, smart-default chart. All surfaces render as intended.
|
||||
- **Bug found + fixed (mid-edit preview resilience).** `filterTransformObject` /
|
||||
`calculateTransformObject` emitted **any non-empty** expression — including a half-typed,
|
||||
unparseable one — so the preview blanked with a raw render error while the user typed.
|
||||
They now **drop a syntactically-invalid expression** like an empty/incomplete entry
|
||||
(guarded by core `validateExpression`), matching `buildTransforms`' own "a config
|
||||
mid-edit still renders" contract; the inline feedback still flags the typo. Pure core,
|
||||
tested (+2 cases). Visually confirmed: an invalid filter/calc now keeps the last-good
|
||||
chart instead of breaking the preview.
|
||||
- **Verified:** `typecheck` + `test` (741 passing) + `eslint` clean.
|
||||
- **Still open (copy judgment, user's call):** the preview's catch-all
|
||||
`"Couldn't render this chart: {raw Vega message}"` puts a diagnostic in the headline
|
||||
(arch 10 says diagnostics go in a disclosure) — correct in the editor, debatable in the
|
||||
builder; and the builder's terse `"Dataset «X» not found."` drops the next-step the
|
||||
contract mandates (near-unreachable in the builder). Both noted, not changed.
|
||||
|
||||
- **2026-06-11** — **1C + 1D + 1E shipped (the Data section).**
|
||||
- A new **Data section** at the top of the builder's left pane — "here are your rows;
|
||||
shape them, then encode them" — emits the spec's top-level `transform` array.
|
||||
- **1C · Filters** — a guarded **field + operator + value** predicate shelf. Operators
|
||||
|
||||
@@ -65,3 +65,15 @@
|
||||
|
||||
- [ ] Library search / sort / empty states, the storage monitor, About & Donate
|
||||
modals, and the busy indicator all look deliberate and behave per spec.
|
||||
|
||||
## Driving these checks headlessly
|
||||
|
||||
The eye-checks above can be scripted as a headless walk-through (Playwright with the
|
||||
already-installed Chromium browser cache) for repeatable screenshots, alongside a live
|
||||
browser.
|
||||
|
||||
- **Select by accessible name / role / placeholder, never by CSS class.** CSS-module class
|
||||
names are content-hashed and unstable; the component tests select the same way.
|
||||
- **Reaching the Chart Builder:** open Datasets from the **header command button** (the
|
||||
pane-toggle-strip `Datasets` button only exists once the workspace has content) → **New
|
||||
Dataset** → paste rows inline → **Build Chart**.
|
||||
|
||||
@@ -35,7 +35,7 @@ The section is ordered **input → shaping** so the distinction reads at a glanc
|
||||
- The **field** dropdown offers the dataset's columns plus any calculated fields.
|
||||
- The **operators** offered depend on the field's type: a measure or temporal field offers `is` / `is not` / `<` / `≤` / `>` / `≥` / `is between` (two bounds); a category offers `is` / `is not` / `is one of` (a comma-separated membership list). A quantitative value compares as a number; other types compare as text (ISO dates sort correctly as text).
|
||||
- A filter can be switched to an **expression** power-mode — a raw Vega predicate expression (e.g. `datum.value > 0`) — for what the guarded shelf can't say. The toggle is reversible.
|
||||
- An **incomplete** filter (no value yet, a blank range bound, an empty expression) is ignored, so the live preview keeps rendering while the user types.
|
||||
- An **incomplete** filter (no value yet, a blank range bound, an empty or syntactically-invalid expression) is ignored, so the live preview keeps rendering while the user types.
|
||||
|
||||
### Calculated fields
|
||||
|
||||
@@ -47,6 +47,7 @@ The section is ordered **input → shaping** so the distinction reads at a glanc
|
||||
|
||||
- Both expression inputs (a filter's expression mode, a calculated field) are validated with **Vega's own expression parser** — the same one the chart uses — so a syntax error is reported **inline** the moment it appears, matching exactly what the chart would accept.
|
||||
- A `datum.<field>` reference that does not match a known column raises a soft **"unknown field"** warning (a typo guard) without blocking — the value is genuinely valid Vega, it just won't resolve.
|
||||
- An expression that does not parse is **left out of the produced spec** (like an incomplete filter or an unnamed calculated field): the inline error is the only feedback, and a half-typed expression never reaches the renderer — the preview keeps showing the last valid chart.
|
||||
|
||||
## Inputs and Controls
|
||||
|
||||
|
||||
@@ -234,11 +234,21 @@
|
||||
|
||||
.exprError,
|
||||
.exprWarn {
|
||||
display: flex;
|
||||
align-items: flex-start;
|
||||
gap: var(--space-2);
|
||||
margin: 0;
|
||||
font-size: 11px;
|
||||
line-height: 1.4;
|
||||
}
|
||||
|
||||
/* Severity reads from the glyph shape + colour, not colour alone (arch 10 §3); the
|
||||
icon inherits the line's colour via currentColor (round error / triangle warning). */
|
||||
.exprFeedbackIcon {
|
||||
flex: none;
|
||||
margin-top: 1px;
|
||||
}
|
||||
|
||||
.exprError {
|
||||
color: var(--support-error);
|
||||
}
|
||||
|
||||
@@ -256,8 +256,15 @@ describe('ChartBuilderModal', () => {
|
||||
await Promise.resolve();
|
||||
});
|
||||
|
||||
const alert = container.querySelector('[role="alert"]');
|
||||
expect(alert?.textContent).toMatch(/nexpected|Invalid/);
|
||||
// Polite, not assertive: live per-keystroke validation uses role="status" with a
|
||||
// status glyph, never an assertive alert (council: APG Alert / WCAG 2.2.4).
|
||||
const messages = Array.from(container.querySelectorAll('[role="status"]'));
|
||||
const errorMsg = messages.find((n) => /nexpected|Invalid/.test(n.textContent ?? ''));
|
||||
expect(errorMsg).toBeTruthy();
|
||||
// The expression input is linked to its message and flagged invalid.
|
||||
const exprInput = container.querySelector('input[aria-label="Filter expression"]');
|
||||
expect(exprInput?.getAttribute('aria-invalid')).toBe('true');
|
||||
expect(exprInput?.getAttribute('aria-describedby')).toBe(errorMsg?.id);
|
||||
});
|
||||
|
||||
test('the Vega expression reference shows only when an expression is in play (1E)', async () => {
|
||||
|
||||
@@ -346,11 +346,26 @@ function ChannelBlock({ channel }: { channel: ChannelName }) {
|
||||
|
||||
/**
|
||||
* Inline feedback for an expression input (filter expression / calculated field):
|
||||
* a parse error (assertive) takes priority, else a soft warning for `datum.<field>`
|
||||
* references that don't match a known column — a typo guard before the chart renders
|
||||
* empty (1E). Nothing renders for a valid, fully-resolved expression.
|
||||
* a parse error takes priority, else a soft warning for `datum.<field>` references
|
||||
* that don't match a known column — a typo guard before the chart renders empty (1E).
|
||||
* Nothing renders for a valid, fully-resolved expression. `messageId` lets the owning
|
||||
* input point at this node via `aria-describedby`.
|
||||
*
|
||||
* Both severities are a **polite** live region carrying a **status glyph** (round
|
||||
* error / triangle warning), not an assertive alert and never colour alone: the
|
||||
* expression validates on every keystroke, so an assertive role would interrupt on
|
||||
* each character (APG Alert / WCAG 2.2.4), and severity must read without colour
|
||||
* (arch 10 §3; the input also carries `aria-invalid`).
|
||||
*/
|
||||
function ExprFeedback({ expr, columns }: { expr: string; columns: BuilderColumns }) {
|
||||
function ExprFeedback({
|
||||
expr,
|
||||
columns,
|
||||
messageId,
|
||||
}: {
|
||||
expr: string;
|
||||
columns: BuilderColumns;
|
||||
messageId?: string;
|
||||
}) {
|
||||
const feedback = useMemo(() => {
|
||||
const validation = validateExpression(expr);
|
||||
if (!validation.valid) {
|
||||
@@ -359,19 +374,23 @@ function ExprFeedback({ expr, columns }: { expr: string; columns: BuilderColumns
|
||||
const unknown = referencedFields(expr).filter((f) => !columns.columns.includes(f));
|
||||
if (unknown.length > 0) {
|
||||
const plural = unknown.length > 1 ? 's' : '';
|
||||
return { kind: 'warn' as const, text: `Unknown field${plural}: ${unknown.join(', ')}` };
|
||||
return {
|
||||
kind: 'warn' as const,
|
||||
text: `Unknown field${plural}: ${unknown.join(', ')} — not a column in this dataset.`,
|
||||
};
|
||||
}
|
||||
return null;
|
||||
}, [expr, columns]);
|
||||
|
||||
if (!feedback) return null;
|
||||
return feedback.kind === 'error' ? (
|
||||
<p className={styles.exprError} role="alert">
|
||||
{feedback.text}
|
||||
</p>
|
||||
) : (
|
||||
<p className={styles.exprWarn} role="status">
|
||||
{feedback.text}
|
||||
const isError = feedback.kind === 'error';
|
||||
return (
|
||||
<p id={messageId} className={isError ? styles.exprError : styles.exprWarn} role="status">
|
||||
<Icon
|
||||
name={isError ? 'status-error' : 'status-warning'}
|
||||
className={styles.exprFeedbackIcon}
|
||||
/>
|
||||
<span>{feedback.text}</span>
|
||||
</p>
|
||||
);
|
||||
}
|
||||
@@ -412,6 +431,9 @@ function FilterRow({ filter, columns }: { filter: BuilderFilter; columns: Builde
|
||||
const op = filter.op ?? 'equal';
|
||||
const arity = filterOpArity(op);
|
||||
const hasColumns = columns.columns.length > 0;
|
||||
// Links the expression input to its feedback line; harmless when no message renders
|
||||
// (aria-describedby to an absent id is ignored — GOV.UK error-message association).
|
||||
const exprMsgId = `filter-${filter.id}-expr-msg`;
|
||||
|
||||
return (
|
||||
<div className={styles.transformBlock}>
|
||||
@@ -423,6 +445,7 @@ function FilterRow({ filter, columns }: { filter: BuilderFilter; columns: Builde
|
||||
placeholder={exprPlaceholder(columns, 'filter')}
|
||||
value={filter.expr ?? ''}
|
||||
aria-invalid={!validateExpression(filter.expr ?? '').valid || undefined}
|
||||
aria-describedby={exprMsgId}
|
||||
onChange={(e) => updateFilter(filter.id, { expr: e.target.value })}
|
||||
/>
|
||||
) : (
|
||||
@@ -496,7 +519,9 @@ function FilterRow({ filter, columns }: { filter: BuilderFilter; columns: Builde
|
||||
</div>
|
||||
)}
|
||||
|
||||
{expressionMode && <ExprFeedback expr={filter.expr ?? ''} columns={columns} />}
|
||||
{expressionMode && (
|
||||
<ExprFeedback expr={filter.expr ?? ''} columns={columns} messageId={exprMsgId} />
|
||||
)}
|
||||
|
||||
{hasColumns && (
|
||||
<button
|
||||
@@ -515,6 +540,7 @@ function FilterRow({ filter, columns }: { filter: BuilderFilter; columns: Builde
|
||||
function CalculateRow({ calc, columns }: { calc: BuilderCalculate; columns: BuilderColumns }) {
|
||||
const updateCalculate = useChartBuilderStore((s) => s.updateCalculate);
|
||||
const removeCalculate = useChartBuilderStore((s) => s.removeCalculate);
|
||||
const exprMsgId = `calc-${calc.id}-expr-msg`;
|
||||
|
||||
return (
|
||||
<div className={styles.transformBlock}>
|
||||
@@ -535,6 +561,7 @@ function CalculateRow({ calc, columns }: { calc: BuilderCalculate; columns: Buil
|
||||
placeholder={exprPlaceholder(columns, 'calc')}
|
||||
value={calc.expr}
|
||||
aria-invalid={!validateExpression(calc.expr).valid || undefined}
|
||||
aria-describedby={exprMsgId}
|
||||
onChange={(e) => updateCalculate(calc.id, { expr: e.target.value })}
|
||||
/>
|
||||
<button
|
||||
@@ -546,7 +573,7 @@ function CalculateRow({ calc, columns }: { calc: BuilderCalculate; columns: Buil
|
||||
<Icon name="close" />
|
||||
</button>
|
||||
</div>
|
||||
<ExprFeedback expr={calc.expr} columns={columns} />
|
||||
<ExprFeedback expr={calc.expr} columns={columns} messageId={exprMsgId} />
|
||||
</div>
|
||||
);
|
||||
}
|
||||
@@ -782,9 +809,15 @@ function BuilderPreview() {
|
||||
setTooLarge({ heightPx: e.heightPx, limitPx: e.limitPx });
|
||||
setError(null);
|
||||
} else if (e instanceof DatasetNotFoundError) {
|
||||
// TODO: this drops the next-step the error contract wants (arch 10); LivePreview
|
||||
// gives "Create it from Datasets…". Near-unreachable here (the builder opens from
|
||||
// an existing dataset), so it's terse — restore the next-step if it can be reached.
|
||||
setError(`Dataset "${e.datasetName}" not found.`);
|
||||
setTooLarge(null);
|
||||
} else {
|
||||
// TODO: arch 10 routes a raw diagnostic into a disclosure, not the headline. The
|
||||
// editor surfaces the Vega message inline by design; the builder could fold it
|
||||
// behind a details disclosure and keep the headline plain.
|
||||
setError(`Couldn't render this chart: ${(e as Error).message}`);
|
||||
setTooLarge(null);
|
||||
}
|
||||
|
||||
@@ -974,6 +974,29 @@ describe('buildTransforms (predicate coercion + shape)', () => {
|
||||
};
|
||||
expect(buildTransforms(config)).toEqual([]);
|
||||
});
|
||||
|
||||
it('drops a syntactically-invalid expression filter (mid-edit preview resilience)', () => {
|
||||
const t = buildTransforms(
|
||||
withFilters(
|
||||
filter({ mode: 'expression', expr: 'datum.value *' }), // half-typed → unparseable
|
||||
filter({ id: 'f2', mode: 'expression', expr: 'datum.value > 0' }), // valid stays
|
||||
),
|
||||
);
|
||||
expect(t).toEqual([{ filter: 'datum.value > 0' }]);
|
||||
});
|
||||
|
||||
it('drops a calculated field whose expression does not parse', () => {
|
||||
const config: BuilderConfig = {
|
||||
datasetName: 'D',
|
||||
mark: 'bar',
|
||||
encodings: {},
|
||||
calculates: [
|
||||
calc({ as: 'bad', expr: 'datum.a +' }),
|
||||
calc({ id: 'c2', as: 'ok', expr: 'datum.a' }),
|
||||
],
|
||||
};
|
||||
expect(buildTransforms(config)).toEqual([{ calculate: 'datum.a', as: 'ok' }]);
|
||||
});
|
||||
});
|
||||
|
||||
describe('buildChartSpec — transform integration', () => {
|
||||
|
||||
@@ -26,6 +26,7 @@ import type { ColumnStats } from './profile';
|
||||
import { DISTINCT_CAP } from './profile';
|
||||
import type { ColumnType } from './type-inference';
|
||||
import { VEGA_LITE_SCHEMA_URL } from './snippet';
|
||||
import { validateExpression } from './expr-validate';
|
||||
|
||||
/** The five mark types the builder offers, in selector order (spec §06). */
|
||||
export const MARK_TYPES = ['bar', 'line', 'point', 'area', 'circle'] as const;
|
||||
@@ -835,25 +836,31 @@ function predicateObject(filter: BuilderFilter): Record<string, unknown> | null
|
||||
function filterTransformObject(filter: BuilderFilter): Record<string, unknown> | null {
|
||||
if (filter.mode === 'expression') {
|
||||
const expr = (filter.expr ?? '').trim();
|
||||
return expr === '' ? null : { filter: expr };
|
||||
// A syntactically-invalid expression is dropped like an empty one: a half-typed
|
||||
// `datum.x *` must not reach the renderer and blank the preview mid-edit — the
|
||||
// inline feedback already flags it. (Same mid-edit resilience as a partial predicate.)
|
||||
return expr === '' || !validateExpression(expr).valid ? null : { filter: expr };
|
||||
}
|
||||
const predicate = predicateObject(filter);
|
||||
return predicate ? { filter: predicate } : null;
|
||||
}
|
||||
|
||||
/** One calculate's `{ calculate, as }` transform entry, or `null` when incomplete. */
|
||||
/** One calculate's `{ calculate, as }` transform entry, or `null` when incomplete or unparseable. */
|
||||
function calculateTransformObject(calc: BuilderCalculate): Record<string, unknown> | null {
|
||||
const expr = calc.expr.trim();
|
||||
const as = calc.as.trim();
|
||||
return expr === '' || as === '' ? null : { calculate: expr, as };
|
||||
return expr === '' || as === '' || !validateExpression(expr).valid
|
||||
? null
|
||||
: { calculate: expr, as };
|
||||
}
|
||||
|
||||
/**
|
||||
* The complete top-level `transform` array for a configuration: every calculated
|
||||
* field first (so filters and encodings can reference the derived columns), then
|
||||
* every filter, each in the user's list order. Incomplete entries (a half-typed
|
||||
* filter, an unnamed calculate) are dropped so a configuration mid-edit still
|
||||
* produces a renderable spec. An empty result means no `transform` key is emitted.
|
||||
* every filter, each in the user's list order. Incomplete or unparseable entries (a
|
||||
* half-typed predicate, an unnamed calculate, an expression that doesn't parse) are
|
||||
* dropped so a configuration mid-edit still produces a renderable spec. An empty
|
||||
* result means no `transform` key is emitted.
|
||||
*/
|
||||
export function buildTransforms(config: BuilderConfig): Array<Record<string, unknown>> {
|
||||
const out: Array<Record<string, unknown>> = [];
|
||||
|
||||
Reference in New Issue
Block a user