From b97267788d735b7f595c20de8a20da6fa6313128 Mon Sep 17 00:00:00 2001 From: Brylie Oxley Date: Sat, 12 Sep 2026 18:39:20 +0300 Subject: [PATCH 1/3] Reject duplicate collection field labels --- src/lib/client/collection-editor.ts | 33 ++++++---- src/lib/components/BoardCollectionView.svelte | 12 +++- .../components/CalendarCollectionView.svelte | 12 +++- src/lib/components/FieldManagerDialog.svelte | 10 +-- .../FieldManagerDialog.svelte.test.ts | 22 ++++++- src/lib/components/FieldMenu.svelte | 16 +++-- ...collection-editing-contract.svelte.test.ts | 65 ++++++++++++++++++- src/lib/data/collection-ops.test.ts | 38 ++++++++++- src/lib/data/collection-ops.ts | 27 ++++++-- 9 files changed, 202 insertions(+), 33 deletions(-) diff --git a/src/lib/client/collection-editor.ts b/src/lib/client/collection-editor.ts index fd75e8f..36bf0fc 100644 --- a/src/lib/client/collection-editor.ts +++ b/src/lib/client/collection-editor.ts @@ -11,7 +11,7 @@ import type { PropertyDefinition, PropertyValue, WorkspaceRecord } from '$lib/da /** * Shared row/cell/select-option/field mutation helpers used by every * Collection renderer — the embedded Table/Board/Calendar views and the - * full-page Table route (issue #189). Each wraps a `$lib/data/records.ts` + * full-page Table route (issue #189). Each wraps a `$lib/data/collection-ops.ts` * primitive with the "no doc yet" guard every renderer previously * reimplemented individually; renderer-specific behavior (Board's * pre-seeded column value, Calendar's pre-seeded date) stays in the @@ -48,9 +48,11 @@ export type SelectOptionResult = | { ok: true; option: { id: string; label: string; color?: string } } | { ok: false; error: string }; +export type CollectionFieldResult = { ok: true } | { ok: false; error: string }; + /** * The one path every renderer must use to add a select option — validated, - * deduped, and palette-colored by `records.ts`'s `addSelectOption`. Before + * deduped, and palette-colored by `collection-ops.ts`'s `addSelectOption`. Before * issue #189, Calendar and the full-page Table route each rebuilt the * schema by hand instead, silently allowing duplicate, uncolored options. */ @@ -76,19 +78,28 @@ export function addCollectionSelectOption( /** * Appends one field to a Collection's schema — the shared path for Board's * "add a select property" and Calendar's "add a date property" first-run - * prompts, and FieldManagerDialog's "Add field" form. Reads the current - * schema from Yjs itself (`records.ts`'s `appendCollectionField`), not from + * prompts, and FieldManagerDialog's "Add field" form. Validates field labels + * while reading the current schema from Yjs itself (`collection-ops.ts`'s + * `appendCollectionField`), not from * a caller-supplied snapshot, so two rapid appends never race. Returns - * whether the field was actually written — `false` when `doc` isn't - * connected yet — so a caller doesn't persist a config referencing a field + * the validation error when it cannot write, so callers can render it inline; + * this also prevents callers from persisting a config referencing a field * (e.g. `groupBy`) that was never added. */ export function appendCollectionField( doc: Y.Doc | undefined, collectionId: string, field: PropertyDefinition -): boolean { - if (!doc) return false; - appendCollectionFieldToSchema(doc, collectionId, field); - return true; -} +): CollectionFieldResult { + if (!doc) return { ok: false, error: 'Not connected yet. Please try again.' }; + try { + appendCollectionFieldToSchema(doc, collectionId, field); + return { ok: true }; + } catch (err) { + return { + ok: false, + error: + err instanceof ValidationError ? err.message : 'Could not add the field. Please try again.' + }; + } +} \ No newline at end of file diff --git a/src/lib/components/BoardCollectionView.svelte b/src/lib/components/BoardCollectionView.svelte index 832d635..e0281e6 100644 --- a/src/lib/components/BoardCollectionView.svelte +++ b/src/lib/components/BoardCollectionView.svelte @@ -52,6 +52,7 @@ let manualOrder: Record = $state({}); let draggedRecordId: string | null = $state(null); let newGroupingPropertyLabel = $state('Status'); + let newGroupingPropertyError = $state(''); let optionDialogPropertyKey: string | null = $state(null); let optionDialogError = $state(''); // The side-pane surface for a card's full schema/backlinks/attribution @@ -188,8 +189,12 @@ const label = newGroupingPropertyLabel.trim(); if (!label) return; const property: PropertyDefinition = { key: nanoid(8), label, type: 'select', options: [] }; - if (appendCollectionField(ydoc, collectionId, property)) { + const result = appendCollectionField(ydoc, collectionId, property); + if (result.ok) { + newGroupingPropertyError = ''; onConfigChange({ ...config, groupBy: property.key }); + } else { + newGroupingPropertyError = result.error; } } @@ -377,6 +382,9 @@ Add a select property + {#if newGroupingPropertyError} + + {/if} {:else}
@@ -603,4 +611,4 @@ onClose={() => (openRecordId = null)} onDelete={removeCard} /> -{/if} +{/if} \ No newline at end of file diff --git a/src/lib/components/CalendarCollectionView.svelte b/src/lib/components/CalendarCollectionView.svelte index dfbc0a7..a5a7fae 100644 --- a/src/lib/components/CalendarCollectionView.svelte +++ b/src/lib/components/CalendarCollectionView.svelte @@ -52,6 +52,7 @@ let viewYear = $state(today.getFullYear()); let viewMonth = $state(today.getMonth()); // 0-11 let newDatePropertyLabel = $state('Date'); + let newDatePropertyError = $state(''); let optionDialogPropertyKey: string | null = $state(null); let optionDialogError = $state(''); // The side-pane surface for an entry's full schema/backlinks/attribution @@ -208,8 +209,12 @@ const label = newDatePropertyLabel.trim(); if (!label) return; const property: PropertyDefinition = { key: nanoid(8), label, type: 'date' }; - if (appendCollectionField(ydoc, collectionId, property)) { + const result = appendCollectionField(ydoc, collectionId, property); + if (result.ok) { + newDatePropertyError = ''; onConfigChange({ ...config, groupBy: property.key }); + } else { + newDatePropertyError = result.error; } } @@ -302,6 +307,9 @@ Add a date property + {#if newDatePropertyError} + + {/if}
{:else}
@@ -532,4 +540,4 @@ onClose={() => (openRecordId = null)} onDelete={removeEntry} /> -{/if} +{/if} \ No newline at end of file diff --git a/src/lib/components/FieldManagerDialog.svelte b/src/lib/components/FieldManagerDialog.svelte index 55a3059..7f3c76f 100644 --- a/src/lib/components/FieldManagerDialog.svelte +++ b/src/lib/components/FieldManagerDialog.svelte @@ -103,14 +103,14 @@ targetCollectionId: newFieldType === 'relation' ? newFieldTargetCollectionId || undefined : undefined }; - try { - appendCollectionField(getShardDoc(shardId), collectionId, field); + const result = appendCollectionField(getShardDoc(shardId), collectionId, field); + if (result.ok) { newFieldLabel = ''; newFieldType = 'text'; newFieldTargetCollectionId = ''; errorMessage = ''; - } catch { - errorMessage = 'Could not add the field. Please try again.'; + } else { + errorMessage = result.error; } } @@ -262,4 +262,4 @@ {/if}
-{/if} +{/if} \ No newline at end of file diff --git a/src/lib/components/FieldManagerDialog.svelte.test.ts b/src/lib/components/FieldManagerDialog.svelte.test.ts index d8268b9..d1e4756 100644 --- a/src/lib/components/FieldManagerDialog.svelte.test.ts +++ b/src/lib/components/FieldManagerDialog.svelte.test.ts @@ -120,6 +120,26 @@ describe('FieldManagerDialog', () => { ]); }); + it('rejects a case-insensitively duplicate field label with an inline error', async () => { + const collection = createCollection(ydoc, { + title: 'T', + schema: [{ key: 'status', label: 'Status', type: 'select' }] + }); + const user = userEvent.setup(); + render(FieldManagerDialog, { + open: true, + collectionId: collection.id, + shardId: 'test-shard', + onClose: vi.fn() + }); + + await user.type(screen.getByPlaceholderText('Field name…'), ' status '); + await user.click(screen.getByRole('button', { name: 'Add field' })); + + expect(screen.getByRole('alert')).toHaveTextContent('A field named "status" already exists'); + expect(getCollection(ydoc, collection.id)?.schema).toHaveLength(1); + }); + it("adds a relation field with a target collection, and doesn't offer the picker for other types (issue #15)", async () => { const people = createCollection(ydoc, { title: 'People', schema: [] }); const collection = createCollection(ydoc, { title: 'T', schema: [] }); @@ -271,4 +291,4 @@ describe('FieldManagerDialog', () => { expect(panel).toHaveClass('z-50'); expect(panel.parentElement).toBe(document.body); }); -}); +}); \ No newline at end of file diff --git a/src/lib/components/FieldMenu.svelte b/src/lib/components/FieldMenu.svelte index abffa2b..b63b5df 100644 --- a/src/lib/components/FieldMenu.svelte +++ b/src/lib/components/FieldMenu.svelte @@ -205,8 +205,11 @@ try { insertCollectionField(getShardDoc(shardId), collectionId, property.key, direction, field); closeMenu(); - } catch { - errorMessage = 'Could not insert a field. Please try again.'; + } catch (err) { + errorMessage = + err instanceof ValidationError + ? err.message + : 'Could not insert a field. Please try again.'; } } @@ -214,8 +217,11 @@ try { duplicateCollectionProperty(getShardDoc(shardId), collectionId, property.key); closeMenu(); - } catch { - errorMessage = 'Could not duplicate the field. Please try again.'; + } catch (err) { + errorMessage = + err instanceof ValidationError + ? err.message + : 'Could not duplicate the field. Please try again.'; } } @@ -795,4 +801,4 @@ {/if} {#if optionError && !open} -{/if} +{/if} \ No newline at end of file diff --git a/src/lib/components/collection-editing-contract.svelte.test.ts b/src/lib/components/collection-editing-contract.svelte.test.ts index e66357e..6c7cf2a 100644 --- a/src/lib/components/collection-editing-contract.svelte.test.ts +++ b/src/lib/components/collection-editing-contract.svelte.test.ts @@ -16,6 +16,7 @@ import { SELECT_OPTION_COLORS } from '$lib/data/select-colors'; import TableCollectionViewHarness from './TableCollectionViewHarness.svelte'; import BoardCollectionViewHarness from './BoardCollectionViewHarness.svelte'; import CalendarCollectionViewHarness from './CalendarCollectionViewHarness.svelte'; +import FieldManagerDialog from './FieldManagerDialog.svelte'; import FullPageTable from '../../routes/space/[spaceId]/table/[id]/+page.svelte'; const actor = { kind: 'human' as const, userId: 'local' }; @@ -206,6 +207,68 @@ describe('cross-surface select-option contract (issue #189)', () => { }); }); +describe('cross-surface field-label validation (issue #205)', () => { + beforeEach(() => { + ydoc = new Y.Doc(); + vi.stubGlobal( + 'fetch', + vi.fn(async () => ({ ok: true, json: async () => ({ shardId: 'test-shard' }) })) + ); + }); + + afterEach(() => { + ydoc.destroy(); + vi.unstubAllGlobals(); + }); + + it('Board and Calendar reject a field label that duplicates another field', async () => { + createCollection(ydoc, { + id: 'board-collection', + title: 'Board', + schema: [{ key: 'existing', label: 'Status', type: 'text' }] + }); + const user = userEvent.setup(); + const { unmount } = render(BoardCollectionViewHarness, { collectionId: 'board-collection' }); + + await user.click(await screen.findByRole('button', { name: 'Add a select property' })); + expect(screen.getByRole('alert')).toHaveTextContent('A field named "Status" already exists'); + expect(getCollection(ydoc, 'board-collection')?.schema).toHaveLength(1); + unmount(); + + createCollection(ydoc, { + id: 'calendar-collection', + title: 'Calendar', + schema: [{ key: 'existing', label: 'Date', type: 'text' }] + }); + render(CalendarCollectionViewHarness, { collectionId: 'calendar-collection' }); + + await user.click(await screen.findByRole('button', { name: 'Add a date property' })); + expect(screen.getByRole('alert')).toHaveTextContent('A field named "Date" already exists'); + expect(getCollection(ydoc, 'calendar-collection')?.schema).toHaveLength(1); + }); + + it('FieldManagerDialog rejects a duplicate label from its Add field form', async () => { + createCollection(ydoc, { + id: 'manager-collection', + title: 'Manager', + schema: [{ key: 'existing', label: 'Status', type: 'text' }] + }); + const user = userEvent.setup(); + render(FieldManagerDialog, { + open: true, + collectionId: 'manager-collection', + shardId: 'test-shard', + onClose: vi.fn() + }); + + await user.type(screen.getByPlaceholderText('Field name…'), 'status'); + await user.click(screen.getByRole('button', { name: 'Add field' })); + + expect(screen.getByRole('alert')).toHaveTextContent('A field named "status" already exists'); + expect(getCollection(ydoc, 'manager-collection')?.schema).toHaveLength(1); + }); +}); + describe('cross-surface row attribution contract (issue #189)', () => { beforeEach(() => { ydoc = new Y.Doc(); @@ -262,4 +325,4 @@ describe('cross-surface row attribution contract (issue #189)', () => { expect(fullPageRow.createdBy).toEqual(tableRow.createdBy); expect(fullPageRow.createdBy).toEqual({ kind: 'human', userId: 'local' }); }); -}); +}); \ No newline at end of file diff --git a/src/lib/data/collection-ops.test.ts b/src/lib/data/collection-ops.test.ts index 07027cf..b08347b 100644 --- a/src/lib/data/collection-ops.test.ts +++ b/src/lib/data/collection-ops.test.ts @@ -148,6 +148,26 @@ describe('appendCollectionField: reads the current Yjs schema atomically (issue 'due' ]); }); + + it('rejects blank and case-insensitively duplicate field labels', () => { + const doc = new Y.Doc(); + const collection = createCollection(doc, { + title: 'Tasks', + schema: [{ key: 'status', label: 'Status', type: 'select' }] + }); + + expect(() => + appendCollectionField(doc, collection.id, { key: 'blank', label: ' ', type: 'text' }) + ).toThrow('Field label cannot be blank'); + expect(() => + appendCollectionField(doc, collection.id, { + key: 'duplicate', + label: ' status ', + type: 'text' + }) + ).toThrow('A field named "status" already exists'); + expect(getCollection(doc, collection.id)?.schema).toHaveLength(1); + }); }); describe('insertCollectionField: reads the current Yjs schema atomically (issue #203)', () => { @@ -191,6 +211,22 @@ describe('insertCollectionField: reads the current Yjs schema atomically (issue }) ).toThrow(NotFoundError); }); + + it('rejects a duplicate field label', () => { + const doc = new Y.Doc(); + const collection = createCollection(doc, { + title: 'Tasks', + schema: [{ key: 'name', label: 'Name', type: 'text' }] + }); + + expect(() => + insertCollectionField(doc, collection.id, 'name', 'right', { + key: 'duplicate', + label: 'name', + type: 'text' + }) + ).toThrow('A field named "name" already exists'); + }); }); describe('moveCollectionField: reads the current Yjs schema atomically (issue #203)', () => { @@ -1000,4 +1036,4 @@ describe('select option lifecycle: add, rename, recolor, reorder, delete (issue expect(getRecord(doc, row.id)?.properties?.status).toEqual({ type: 'select', value: 'todo' }); }); }); -}); +}); \ No newline at end of file diff --git a/src/lib/data/collection-ops.ts b/src/lib/data/collection-ops.ts index 773890f..4cde5a7 100644 --- a/src/lib/data/collection-ops.ts +++ b/src/lib/data/collection-ops.ts @@ -70,13 +70,24 @@ export function updateCollectionSchema(doc: Y.Doc, id: string, schema: PropertyD ymeta.set('schema', schema); } -/** Appends one field to a Collection's schema, reading the current schema from Yjs inside the same transaction rather than trusting a caller-supplied snapshot — two rapid appends from the same reactive snapshot would otherwise race, with the second silently dropping the first. */ +function assertUniqueFieldLabel(schema: PropertyDefinition[], label: string): string { + const trimmed = label.trim(); + if (!trimmed) throw new ValidationError('Field label cannot be blank'); + const collides = schema.some( + (property) => property.label.trim().toLowerCase() === trimmed.toLowerCase() + ); + if (collides) throw new ValidationError(`A field named "${trimmed}" already exists`); + return trimmed; +} + +/** Appends one field to a Collection's schema, rejecting a blank or already-used (case-insensitive) label. Reads the current schema from Yjs inside the same transaction rather than trusting a caller-supplied snapshot — two rapid appends from the same reactive snapshot would otherwise race, with the second silently dropping the first. */ export function appendCollectionField(doc: Y.Doc, id: string, field: PropertyDefinition): void { const ymeta = collectionsMap(doc).get(id); if (!ymeta) throw new NotFoundError(`Collection ${id} not found`); doc.transact(() => { const schema = ymeta.get('schema') ?? []; - ymeta.set('schema', [...schema, field]); + const label = assertUniqueFieldLabel(schema, field.label); + ymeta.set('schema', [...schema, { ...field, label }]); }); } @@ -99,8 +110,13 @@ export function insertCollectionField( const schema = ymeta.get('schema') ?? []; const index = schema.findIndex((p) => p.key === referenceKey); if (index === -1) throw new NotFoundError(`Property ${referenceKey} not found`); + const label = assertUniqueFieldLabel(schema, field.label); const insertAt = direction === 'left' ? index : index + 1; - ymeta.set('schema', [...schema.slice(0, insertAt), field, ...schema.slice(insertAt)]); + ymeta.set('schema', [ + ...schema.slice(0, insertAt), + { ...field, label }, + ...schema.slice(insertAt) + ]); }); } @@ -356,10 +372,11 @@ export function duplicateCollectionProperty( const index = schema.findIndex((p) => p.key === propertyKey); if (index === -1) throw new NotFoundError(`Property ${propertyKey} not found`); const source = schema[index]; + const label = assertUniqueFieldLabel(schema, `${source.label} copy`); const copy: PropertyDefinition = { ...source, key: nanoid(8), - label: `${source.label} copy`, + label, options: source.options?.map((o) => ({ ...o })) }; const nextSchema = [...schema.slice(0, index + 1), copy, ...schema.slice(index + 1)]; @@ -728,4 +745,4 @@ export function deleteCollection(doc: Y.Doc, id: string): void { } collectionsMap(doc).delete(id); }); -} +} \ No newline at end of file From 2c37eb84613a54c9d38ef61b3e9413b7cb3b08fe Mon Sep 17 00:00:00 2001 From: Brylie Oxley Date: Sat, 12 Sep 2026 18:49:46 +0300 Subject: [PATCH 2/3] Harden field label validation --- docs/specifications/collection-views.md | 6 +- src/lib/components/BoardCollectionView.svelte | 11 ++- .../components/CalendarCollectionView.svelte | 10 ++- src/lib/components/FieldManagerDialog.svelte | 6 +- .../FieldManagerDialog.svelte.test.ts | 20 ++++- src/lib/components/FieldMenu.svelte | 16 ++-- src/lib/components/FieldMenu.svelte.test.ts | 73 +++++++++++++++++++ ...collection-editing-contract.svelte.test.ts | 30 +++++++- src/lib/data/collection-ops.test.ts | 68 ++++++++++++++++- src/lib/data/collection-ops.ts | 50 ++++++++++--- 10 files changed, 255 insertions(+), 35 deletions(-) diff --git a/docs/specifications/collection-views.md b/docs/specifications/collection-views.md index f05e6ac..4c5d527 100644 --- a/docs/specifications/collection-views.md +++ b/docs/specifications/collection-views.md @@ -52,7 +52,7 @@ Each is a standalone component (`src/lib/components/*CollectionView.svelte`) tak **Board swimlanes** (`config.swimlaneBy`, issue #67/#165) — an optional second grouping dimension, rows crossing the existing columns, GitLab/Jira-style. A "Swimlane by" dropdown next to "Group by" offers every `select` property in the schema except whichever one is already `groupBy` (Board never lets one property drive both dimensions); it's hidden entirely when no other `select` property exists, rather than prompting to create one the way the initial "Group by" empty state does — the swimlane dimension is optional, not required to use Board at all. When set, `groupBySwimlaneAndColumn` (§3) replaces the flat column render with one row per swimlane (plus a trailing "No ``" catch-all), each rendering the same full column set — every column still appears inside every swimlane, including an otherwise-empty one, matching `groupBySelectProperty`'s own "preserve empty groups" rule at the second dimension. Both the "Move to column" `` are per-card, keyboard/screen-reader-accessible alternatives to drag-and-drop; dragging a card into a different swimlane's column cell (or dropping it via native drag-and-drop) sets both the column and swimlane grouping properties in one `setCollectionCell` call, and the swimlane-only move sets just the swimlane property, leaving the record's column untouched. Manual per-cell card order (§8) is scoped by swimlane too, keyed by `` `${groupBy}:${swimlaneBy}:${swimlaneOptionId}:${columnOptionId}` `` — switching either grouping property, or moving a card across swimlanes, never resurrects a stale order saved under a different cell. Retargeting "Group by" onto the property currently driving swimlanes clears `swimlaneBy` (rather than leaving it pointing at what's now the column property too — a duplicate-dimension state `swimlaneCandidates` can never resolve back to) as part of that same `onConfigChange` call; a different retarget leaves `swimlaneBy` untouched. If `swimlaneBy` ever names a property that's been deleted out from under it instead, the swimlane row disappears and Board falls back to its flat single-dimension column view — the same graceful-degradation `groupProperty` itself already has when `groupBy` names a missing field. -**Calendar** (`CalendarCollectionView.svelte`) — `config.groupBy` names the `date` property driving placement, same add-one-if-missing prompt as Board. A fixed 6-row/42-cell month grid (leading/trailing days from adjacent months included, so the grid's shape doesn't jump between 5- and 6-row months) places each record on the day matching `dateKeyForRecord`; a record with no value for the date property renders in an "Unscheduled" section below rather than being hidden. Every entry (scheduled or unscheduled) shows the Collection's resolved primary field (§7) as its title — normally a directly-editable `PropertyValueCell`, matching Board, rendered `compact` given Calendar's denser rows — plus its date property (also inline via `PropertyValueCell`, so rescheduling, including giving an unscheduled record its first date, is a direct edit) and whatever other schema properties are in `visibleProperties` (issue #105: this predates #96 as a plain-text-only title and was made editable to match Board once "primary field" became a named concept applied identically everywhere else — Calendar's entries already supported inline editing for the date field, so this isn't a new interaction pattern for the view, just extending it to one more field). A day cell's "+" button creates a record with that day pre-filled (`createCollectionRow`). +**Calendar** (`CalendarCollectionView.svelte`) — `config.groupBy` names the `date` property driving placement, same add-one-if-missing prompt as Board. A fixed 6-row/42-cell month grid (leading/trailing days from adjacent months included, so the grid's shape doesn't jump between 5- and 6-row months) places each record on the day matching `dateKeyForRecord`; a record with no value for the date property renders in an "Unscheduled" section below rather than being hidden. Every entry (scheduled or unscheduled) renders its date property inline via `PropertyValueCell`, so rescheduling — including giving an unscheduled record its first date — is a direct edit. A day cell's "+" button creates a record with that day pre-filled (`createCollectionRow`). ## 5. Permission scoping and `get_document` @@ -64,6 +64,8 @@ Each is a standalone component (`src/lib/components/*CollectionView.svelte`) tak Both new components call `src/lib/data/records.ts` directly against the browser's own `Y.Doc`, the same direct-UI-mutation pattern every other schema edit in this codebase uses (`updateCollectionSchema` from `+page.svelte` predates this feature) — not routed through the service layer, and picked up generically by `attachDocAuditObserver` (`audit-coverage.md`) like any other direct UI write. +**Field-label contract.** Every field label is trimmed before storage, must be non-blank, and must be unique case-insensitively within its Collection schema. The Add field form, Board's initial select-property prompt, Calendar's initial date-property prompt, and FieldMenu rename all surface a rejected label as an inline `ValidationError`; no schema or view configuration is written on rejection. FieldMenu's generated actions choose an available label (`New field`, then `New field 2`, and `