diff --git a/internal/items/lane_write_bug3057_test.go b/internal/items/lane_write_bug3057_test.go new file mode 100644 index 000000000..6bec16095 --- /dev/null +++ b/internal/items/lane_write_bug3057_test.go @@ -0,0 +1,80 @@ +package items + +import ( + "strings" + "testing" + + "github.com/PerpetualSoftware/pad/internal/models" +) + +// The server half of BUG-3057's contract. +// +// A board/list lane key is a STRING projection of a field value, and the web +// write paths used to assign it straight back to the field. `laneWriteValue` +// (web/src/lib/collections/laneWriteValue.ts) now converts through the declared +// type, and its comments cite what this validator does with each form. That +// citation is only worth anything if it is pinned HERE: a change to the +// validator that started accepting the string forms, or stopped accepting the +// converted ones, would leave that module correct against a validator that has +// moved and nothing would notice. +// +// Not a test of the fix — the fix is in TypeScript. A test of the facts the fix +// was derived from. +func TestLaneKeyWrites_BUG3057(t *testing.T) { + schema := models.CollectionSchema{Fields: []models.FieldDef{ + {Key: "score", Type: "number"}, + {Key: "shipped", Type: "checkbox"}, + {Key: "tags", Type: "multi_select", Options: []string{"a", "b", "c"}}, + {Key: "stage", Type: "select", Options: []string{"open", "done"}}, + }} + + cases := []struct { + name string + patch map[string]any + wantErr string // "" means the write must be ACCEPTED + }{ + // What the lane key IS, pre-fix. Every one of these is a legitimate + // drag or create-in-lane that failed. + {"number gets the raw lane key", map[string]any{"score": "0"}, "must be a number"}, + {"number gets the uncategorised lane key", map[string]any{"score": ""}, "must be a number"}, + {"checkbox gets the raw lane key", map[string]any{"shipped": "false"}, "must be a boolean"}, + {"multi_select gets the raw lane key", map[string]any{"tags": "c"}, "must be an array of strings"}, + + // What `laneWriteValue` sends instead. + {"number gets a number", map[string]any{"score": float64(0)}, ""}, + {"number cleared by the delete sentinel", map[string]any{"score": nil}, ""}, + {"checkbox gets a bool", map[string]any{"shipped": false}, ""}, + {"checkbox cleared by the delete sentinel", map[string]any{"shipped": nil}, ""}, + {"multi_select gets a one-element array", map[string]any{"tags": []any{"c"}}, ""}, + // A lane is a COMBINATION, so a two-tag lane writes two tags — and both + // have to be declared options or this arm refuses, which is exactly why + // the web side resolves the lane key against the options rather than + // splitting it blindly. + {"multi_select gets a two-element array", map[string]any{"tags": []any{"a", "b"}}, ""}, + {"multi_select given the JOINED lane key as one tag", map[string]any{"tags": []any{"a,b"}}, "not in allowed options"}, + {"multi_select cleared by an empty array", map[string]any{"tags": []any{}}, ""}, + + // The case that was never broken, pinned so a conversion applied to + // EVERY type would be caught from this side too. + {"select keeps taking its lane key", map[string]any{"stage": "done"}, ""}, + {"select cleared by the empty lane key", map[string]any{"stage": ""}, ""}, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + err := ValidatePartialFields(tc.patch, schema) + if tc.wantErr == "" { + if err != nil { + t.Fatalf("write must be accepted, got: %v", err) + } + return + } + if err == nil { + t.Fatalf("write must be refused (%s), got nil — laneWriteValue's premise has moved", tc.wantErr) + } + if !strings.Contains(err.Error(), tc.wantErr) { + t.Fatalf("refused for the wrong reason: want %q, got %v", tc.wantErr, err) + } + }) + } +} diff --git a/web/src/lib/collections/laneKeyCallers.test.ts b/web/src/lib/collections/laneKeyCallers.test.ts index db140436b..e18892276 100644 --- a/web/src/lib/collections/laneKeyCallers.test.ts +++ b/web/src/lib/collections/laneKeyCallers.test.ts @@ -137,3 +137,62 @@ describe('no surface keeps a private copy of a lane-key helper', () => { ).toBe(true); }); }); + +/** + * The WRITE side of the same seam (BUG-3057). + * + * BUG-3053 closed the read side — comparing a lane key against a raw value — + * and this file's guards are about surfaces READING through the shared helpers. + * The two write paths on the collection page had the mirror defect: they + * assigned the lane KEY back to the field, whatever its declared type, and the + * server refuses those writes (`"0"` into a number, `"c"` into a multi_select), + * so a legitimate drag or create-in-lane simply failed. + * + * Asserted at the SOURCE for the reason the guards above are: a behaviour test + * of `laneWriteValue` stays green if a call site stops calling it, and the + * call sites live in a 2000-line route component whose write paths need a + * board, a drag and a network mock to reach. + */ +describe('BUG-3057: the collection page writes a CONVERTED lane value', () => { + const page = read('../../routes/[username]/[workspace]/[collection]/+page.svelte'); + + it('imports the converter', () => { + expect(page).toContain("from '$lib/collections/laneWriteValue'"); + expect(page).toContain('laneWriteValue'); + expect(page).toContain('laneWriteRefusalMessage'); + }); + + it('sends the CONVERTED value on the drag path, not the lane key', () => { + // The mutant this kills is the original line, restored. The patched key + // is `fieldKey` rather than `groupField` because the writer now takes its + // target explicitly — see the status-chip leg below. + expect(page).toContain('const fieldsPatch = { [fieldKey]: laneWrite.value };'); + expect(page).not.toContain('const fieldsPatch = { [groupField]: newValue };'); + }); + + it('lets the table status chip name the STATUS field, not the lane field', () => { + // The table has no lanes: its chip cycles the `status` schema field's + // options and used to send them to `groupField`, which in table view is + // `list_group_by`. On a table grouped by `priority`, a status click set + // the priority. The default argument keeps every drag caller on the lane + // field, so this leg is what says the table opted out of it. + expect(page).toContain( + "onStatusChange={(it, newStatus) => handleStatusChange(it, newStatus, 'status')}", + ); + expect(page).toContain( + 'async function handleStatusChange(item: Item, newValue: string, fieldKey: string = groupField)', + ); + }); + + it('sends the CONVERTED value on the create-in-lane path, not the lane key', () => { + expect(page).toContain('laneWriteValue(groupFieldDef, groupValue)'); + expect(page).not.toContain('defaultFields[groupField] = groupValue;'); + }); + + it('resolves the group field against the SCHEMA, since the type is what converts', () => { + // Without this derive, both call sites would have nothing to convert + // through and the converter would fall into its undeclared-field arm — + // which writes the key, i.e. the defect, with a helper in front of it. + expect(page).toContain("let groupFieldDef = $derived(schema?.fields.find((f) => f.key === groupField));"); + }); +}); diff --git a/web/src/lib/collections/laneWriteValue.test.ts b/web/src/lib/collections/laneWriteValue.test.ts new file mode 100644 index 000000000..8c5c9bea8 --- /dev/null +++ b/web/src/lib/collections/laneWriteValue.test.ts @@ -0,0 +1,126 @@ +// Node-project test: a lane key becomes a value the schema can hold (BUG-3057). +// +// The pairing that matters is with `internal/items`: every value this returns +// has to be one the server's validator ACCEPTS for that type, and every string +// it replaces is one the validator REFUSES. The Go side pins the same table +// from its end (TestLaneKeyWrites_BUG3057), so the two cannot drift into a +// state where this module is "correct" against a validator that has moved. +import { describe, it, expect } from 'vitest'; +import { laneWriteValue, laneWriteRefusalMessage } from './laneWriteValue'; +import type { FieldDef } from '$lib/types'; + +const f = (type: string, over: Partial = {}): FieldDef => + ({ key: 'g', label: 'Group', type, ...over }) as unknown as FieldDef; + +describe('laneWriteValue', () => { + it('passes a string-shaped field its lane key unchanged', () => { + // select is the common case and was never broken; it is here so a fix + // that converted everything would be caught. + expect(laneWriteValue(f('select'), 'done')).toEqual({ ok: true, value: 'done' }); + expect(laneWriteValue(f('text'), 'done')).toEqual({ ok: true, value: 'done' }); + expect(laneWriteValue(f('date'), '2026-01-01')).toEqual({ ok: true, value: '2026-01-01' }); + // The established clear for a select — `''` is a legal write there. + expect(laneWriteValue(f('select'), '')).toEqual({ ok: true, value: '' }); + }); + + it('gives a number field a NUMBER, including zero', () => { + // `0` is the case BUG-3053 was about on the read side: it is an ordinary + // value with an ordinary lane, and `'0'` is what the projection makes of + // it. The write has to undo exactly that. + expect(laneWriteValue(f('number'), '0')).toEqual({ ok: true, value: 0 }); + expect(laneWriteValue(f('number'), '10')).toEqual({ ok: true, value: 10 }); + expect(laneWriteValue(f('number'), '-2.5')).toEqual({ ok: true, value: -2.5 }); + }); + + it('clears a number field with the DELETE sentinel, not an empty string', () => { + // `''` is what the uncategorised lane means, and the validator refuses it + // for a number ("must be a number"). `null` is the patch path's delete. + expect(laneWriteValue(f('number'), '')).toEqual({ ok: true, value: null }); + }); + + it('refuses a number lane key that is not a number', () => { + // Reachable when a field is retyped under a `board_group_by` that still + // names it: the lanes are still the old options. + expect(laneWriteValue(f('number'), 'open')).toEqual({ ok: false, reason: 'not_a_number' }); + // Whitespace is not zero. `Number(' ')` is 0, which would turn a blank + // lane into a real value. + expect(laneWriteValue(f('number'), ' ')).toEqual({ ok: false, reason: 'not_a_number' }); + expect(laneWriteValue(f('number'), 'NaN')).toEqual({ ok: false, reason: 'not_a_number' }); + expect(laneWriteValue(f('number'), 'Infinity')).toEqual({ ok: false, reason: 'not_a_number' }); + }); + + it('gives a checkbox field a BOOLEAN, and only for the two spellings the projection makes', () => { + expect(laneWriteValue(f('checkbox'), 'true')).toEqual({ ok: true, value: true }); + expect(laneWriteValue(f('checkbox'), 'false')).toEqual({ ok: true, value: false }); + expect(laneWriteValue(f('checkbox'), '')).toEqual({ ok: true, value: null }); + // Not guesses: nothing in this seam was typed by a user, so 'yes' / '1' / + // 'on' would be inventing an intent. + expect(laneWriteValue(f('checkbox'), '1')).toEqual({ ok: false, reason: 'not_a_boolean' }); + expect(laneWriteValue(f('checkbox'), 'yes')).toEqual({ ok: false, reason: 'not_a_boolean' }); + }); + + describe('multi_select — a lane is a COMBINATION, so the write inverts the projection', () => { + // The picker OFFERS multi_select, so all of this is reachable with no + // retype. `laneValue(['a','b'])` is `'a,b'` and ListView mints a lane per + // distinct value, so an `a,b` lane really exists on a grouped list + // (measured; the board seeds lanes from options, so that item sits in + // UNCATEGORIZED there instead). + const tags = f('multi_select', { options: ['a', 'b', 'c'] }); + + it('writes BOTH tags for a two-tag lane', () => { + // The defect this leg exists for: wrapping the key wrote the single + // tag `"a,b"`, which is not an option and not a tag anyone has. + expect(laneWriteValue(tags, 'a,b')).toEqual({ ok: true, value: ['a', 'b'] }); + }); + + it('CONTROL: a single-tag lane still writes one tag', () => { + // Without this, "always split on comma" and "always wrap" both pass + // the leg above for the wrong reason on one side or the other. + expect(laneWriteValue(tags, 'c')).toEqual({ ok: true, value: ['c'] }); + }); + + it('does not TEAR an option that contains a comma', () => { + // Rule 1 (the key IS an option) before rule 2 (every part is an + // option). A blind split would write two tags that do not exist. + const commas = f('multi_select', { options: ['a,b', 'c'] }); + expect(laneWriteValue(commas, 'a,b')).toEqual({ ok: true, value: ['a,b'] }); + }); + + it('prefers the REAL option when the lane is genuinely ambiguous', () => { + // Options `['a,b','a','b']` make the lane `a,b` unreadable in + // principle — the projection is lossy — so the tie-break is stated + // rather than left to whichever rule runs first: the reading that + // names an option that EXISTS wins. + const ambiguous = f('multi_select', { options: ['a,b', 'a', 'b'] }); + expect(laneWriteValue(ambiguous, 'a,b')).toEqual({ ok: true, value: ['a,b'] }); + }); + + it('splits when the field declares NO options', () => { + // Nothing to check a part against, and splitting is the plain inverse + // of the join that made the lane. + expect(laneWriteValue(f('multi_select'), 'a,b')).toEqual({ ok: true, value: ['a', 'b'] }); + }); + + it('keeps an unrecognised lane whole', () => { + // A lane minted from a value that is not in the vocabulary at all: + // the honest write is the value the lane was named for. + expect(laneWriteValue(tags, 'legacy,x')).toEqual({ ok: true, value: ['legacy,x'] }); + }); + + it('clears with an empty array', () => { + expect(laneWriteValue(tags, '')).toEqual({ ok: true, value: [] }); + }); + }); + + it('writes the key for a field the schema does not declare', () => { + // Orphan keys are accepted server-side and persist as-is; with no type + // there is nothing to convert through, and today's behaviour is right. + expect(laneWriteValue(undefined, 'anything')).toEqual({ ok: true, value: 'anything' }); + expect(laneWriteValue(null, 'anything')).toEqual({ ok: true, value: 'anything' }); + }); + + it('names the field in a refusal message', () => { + expect(laneWriteRefusalMessage('not_a_number', 'Score')).toContain('Score'); + expect(laneWriteRefusalMessage('not_a_boolean', 'Shipped')).toContain('Shipped'); + }); +}); diff --git a/web/src/lib/collections/laneWriteValue.ts b/web/src/lib/collections/laneWriteValue.ts new file mode 100644 index 000000000..cfdf6c510 --- /dev/null +++ b/web/src/lib/collections/laneWriteValue.ts @@ -0,0 +1,134 @@ +// Turning a board/list LANE KEY back into a field VALUE (BUG-3057). +// +// A lane key is a STRING — `laneValue` in `boardColumns` projects any stored +// value onto one, which is what makes a view able to group by anything. The +// write paths then assigned that string straight back to the field, whatever +// the field's declared type. That projection has no free inverse: `'0'` came +// from `0`, but the key alone cannot say whether the field wanted `0`, `'0'` or +// `false`. The declared TYPE is what disambiguates, so the conversion belongs +// here rather than inline at either call site. +// +// WHAT THE SERVER ACTUALLY DOES, measured rather than assumed (probe against +// `internal/items.ValidatePartialFields`, the validator behind `fields_patch`): +// +// number "0" -> field "score" must be a number +// number "" -> field "score" must be a number +// number nil -> ok (nil is the patch path's DELETE sentinel) +// checkbox "false" -> field "shipped" must be a boolean +// multi_select "c" -> field "tags" must be an array of strings +// multi_select ["c"] -> ok +// multi_select [] -> ok +// select "" -> ok (clearing a select is a legal write) +// +// So the pre-fix symptom is NOT the silent retyping the item was filed as: the +// server REFUSES these writes. A legitimate drag between lanes, and a +// legitimate create-in-lane, simply FAIL on any board whose group field is not +// string-shaped. Nothing is corrupted; the user's move does not take. +// +// WHICH TYPES ARE REACHABLE. The display-settings picker offers `select` AND +// `multi_select` (EditCollectionModal's `selectFieldKeys`), so a multi_select +// board is reachable with no retype at all. Everything else arrives the way +// BUG-3041 and BUG-3016's cases arrive — a field retyped under a setting that +// still names it, or a `board_group_by` written through the API. +import type { FieldDef } from '$lib/types'; +import { UNCATEGORIZED } from './boardColumns'; + +/** + * Why a lane key cannot become a value for this field. + * + * Only reachable when the lane key and the declared type disagree about what + * the field holds — a stale lane from before a retype, or a `board_group_by` + * pointing at a field whose values were never lane keys. + */ +export type LaneWriteRefusal = 'not_a_number' | 'not_a_boolean'; + +export type LaneWrite = { ok: true; value: unknown } | { ok: false; reason: LaneWriteRefusal }; + +/** + * The value to write for a drop into `laneKey`, or a refusal. + * + * UNCATEGORIZED (`''`) means "no value", and the honest spelling of that + * differs by type: `''` for a string-shaped field (a legal, established clear — + * `boardColumns` documents the empty lane as writing it back), `[]` for a + * multi_select, and the patch path's DELETE sentinel `null` for a number or a + * checkbox, which have no empty string to hold. + * + * An UNDECLARED field (no schema entry) keeps today's behaviour and writes the + * key: orphan keys are accepted and persist as-is server-side, and this module + * has nothing better to go on than the string it was handed. + */ +export function laneWriteValue(field: FieldDef | undefined | null, laneKey: string): LaneWrite { + const type = field?.type; + if (!field || !type) return { ok: true, value: laneKey }; + + switch (type) { + case 'number': { + if (laneKey === UNCATEGORIZED) return { ok: true, value: null }; + // `Number('')` is 0 and `Number(' ')` is 0, which would turn an + // unlabelled lane into a real zero; the empty case is handled above + // and whitespace is not a number anyone typed. + const n = laneKey.trim() === '' ? NaN : Number(laneKey); + if (!Number.isFinite(n)) return { ok: false, reason: 'not_a_number' }; + return { ok: true, value: n }; + } + case 'checkbox': { + if (laneKey === UNCATEGORIZED) return { ok: true, value: null }; + // Exactly the two spellings `laneValue(true)` / `laneValue(false)` + // produce. Nothing else is accepted: 'yes', '1' and 'on' are guesses + // about what a user meant, and this seam never had a user typing. + if (laneKey === 'true') return { ok: true, value: true }; + if (laneKey === 'false') return { ok: true, value: false }; + return { ok: false, reason: 'not_a_boolean' }; + } + case 'multi_select': { + // A LANE IS A COMBINATION, so the write is the combination — which + // makes this the one arm that has to INVERT the projection rather + // than wrap the key. + // + // `laneValue(['a','b'])` is `'a,b'`, and ListView MINTS a lane for + // every distinct value it finds, so a list grouped by a multi_select + // really does show an `a,b` lane (measured; the board instead seeds + // its lanes from the field's options, so the same item lands in + // UNCATEGORIZED there). Dropping onto that lane means "give this item + // that lane's tags". Wrapping the key would have written the single + // tag `"a,b"`, which is not an option and not a tag anyone has. + // + // Resolved against the DECLARED OPTIONS rather than split blindly, + // so an option that CONTAINS a comma is not torn in half: + // 1. the key IS an option -> that one option + // 2. every comma-part is an option -> those options, in lane order + // 3. no options declared -> split, the plain inverse of the + // join, since there is no vocabulary to check against + // 4. otherwise -> the key as a single value, which + // is what a lane minted from an undeclared value means + // + // Rule 1 before rule 2 is a deliberate tie-break: with options + // `['a,b','a','b']` the lane `a,b` is genuinely ambiguous — the + // projection is lossy — and the reading that names a REAL option beats + // the one that reconstructs two. + if (laneKey === UNCATEGORIZED) return { ok: true, value: [] }; + const options = field.options ?? []; + if (options.length === 0) { + return { ok: true, value: laneKey.split(',').filter((p) => p !== '') }; + } + if (options.includes(laneKey)) return { ok: true, value: [laneKey] }; + const parts = laneKey.split(','); + if (parts.length > 1 && parts.every((p) => options.includes(p))) { + return { ok: true, value: parts }; + } + return { ok: true, value: [laneKey] }; + } + default: + // `select`, `text`, `url`, `date`, `relation`, `json` — all hold a + // string, and a relation lane key is already the target's item id + // (the relation lane path resolves it before it gets here). + return { ok: true, value: laneKey }; + } +} + +/** What to tell the user when a lane key cannot become a value. */ +export function laneWriteRefusalMessage(reason: LaneWriteRefusal, fieldLabel: string): string { + return reason === 'not_a_number' + ? `Can't move: ${fieldLabel} holds a number and this lane isn't one` + : `Can't move: ${fieldLabel} holds a checkbox and this lane isn't true or false`; +} diff --git a/web/src/routes/[username]/[workspace]/[collection]/+page.svelte b/web/src/routes/[username]/[workspace]/[collection]/+page.svelte index 6a4f37ef6..b027b563d 100644 --- a/web/src/routes/[username]/[workspace]/[collection]/+page.svelte +++ b/web/src/routes/[username]/[workspace]/[collection]/+page.svelte @@ -8,6 +8,7 @@ import { parseSettings, parseFields, parseSchema, parseTags, getStatusOptions, itemUrlId, formatItemRef } from '$lib/types'; import { plansProgressToMap, fetchCollectionProgress } from '$lib/collections/progressMerge'; import { resolveRenameNavTarget, resolveSyncRenameTarget } from '$lib/collections/renameNav'; + import { laneWriteValue, laneWriteRefusalMessage } from '$lib/collections/laneWriteValue'; import BoardView from '$lib/components/collections/BoardView.svelte'; import ListView from '$lib/components/collections/ListView.svelte'; import TableView from '$lib/components/collections/TableView.svelte'; @@ -1315,6 +1316,14 @@ : (settings?.list_group_by ?? 'status') ); + /** + * The group field's SCHEMA entry — what a lane key has to be converted back + * through before it is written (BUG-3057). `undefined` for a `board_group_by` + * naming a field the schema does not declare, which `laneWriteValue` treats + * as "write the key", matching the server's orphan-key handling. + */ + let groupFieldDef = $derived(schema?.fields.find((f) => f.key === groupField)); + let statusOptions = $derived(collection ? getStatusOptions(collection) : []); /** Schema keys whose filter value names an item rather than an option. */ @@ -1665,7 +1674,24 @@ searchResultRank = new Map(hits.map((h, i) => [h.id, i])); }); - async function handleStatusChange(item: Item, newValue: string) { + /** + * Write one field on one item, from a lane move or from a status chip. + * + * `fieldKey` is EXPLICIT because two different intents arrive here and they + * do not name the same field (BUG-3057, codex enumeration round). A drag + * between lanes means "put this item in THIS LANE" — the group field. A + * status chip means "set this item's STATUS" — the `status` field and + * nothing else. With `groupField` hardcoded, the TABLE's status chip, whose + * options come from the `status` schema field, wrote its value into whatever + * `list_group_by` named: on a table grouped by `priority`, clicking a status + * chip set the priority. Defaulting to `groupField` leaves every drag caller + * unchanged. + * + * The same conflation exists for the status chip on a LIST/BOARD card, where + * one prop serves both the drag and the chip — that needs a second callback + * through those components rather than an argument here, and is BUG-3068. + */ + async function handleStatusChange(item: Item, newValue: string, fieldKey: string = groupField) { if (!wsSlug) return; // BUG-3049: patch ONLY the group field. This used to read the item's // fields, set one key and send the whole blob back as a full replace, @@ -1673,7 +1699,24 @@ // reverted. `fields_patch` merges per key under the row lock // (internal/store/items.go::mergeFieldsPatch), so no other field is // reachable by this write at all. - const fieldsPatch = { [groupField]: newValue }; + // BUG-3057: a lane key is a STRING projection of the value, and assigning + // it straight back sent a string to a field the schema says holds + // something else. The server REFUSES those writes (`"0"` into a number, + // `"c"` into a multi_select), so a legitimate drag between lanes simply + // failed on any board not grouped by a string-shaped field. Convert + // through the declared type; see `laneWriteValue` for the measured + // server behaviour behind each case. + const targetFieldDef = schema?.fields.find((f) => f.key === fieldKey); + const laneWrite = laneWriteValue(targetFieldDef, newValue); + if (!laneWrite.ok) { + // Thrown, not swallowed: BoardView unwinds its optimistic reorder on + // a rejection, which is the same contract the catch below relies on. + const label = targetFieldDef?.label || fieldKey; + const msg = laneWriteRefusalMessage(laneWrite.reason, label); + toastStore.show(msg, 'error'); + throw new Error(msg); + } + const fieldsPatch = { [fieldKey]: laneWrite.value }; const ws = wsSlug; const parentRef = formatItemRef(item) ?? item.slug; @@ -1932,8 +1975,21 @@ defaultFields.status = statusField.options[0]; } // Pre-fill the lane's group field (status, or a custom - // board_group_by select) so the item opens in this lane. - defaultFields[groupField] = groupValue; + // board_group_by select) so the item opens in this lane — converted + // through the declared type, for the reason on the drag path + // (BUG-3057). Creating in a `0` lane of a number field used to send + // the string `"0"`, which the server refuses, so the create failed. + const laneWrite = laneWriteValue(groupFieldDef, groupValue); + if (!laneWrite.ok) { + const label = groupFieldDef?.label || groupField; + toastStore.show(laneWriteRefusalMessage(laneWrite.reason, label), 'error'); + return null; + } + // A null here is the PATCH path's delete sentinel and means nothing on + // create — the field is simply absent from a new item. + if (laneWrite.value !== null) { + defaultFields[groupField] = laneWrite.value; + } // Epoch before create: a brand-new id is never in the fence set, // so this is the case the epoch guard exists for (BUG-2098). const epoch = localIndex.scopeEpochFor(wsSlug); @@ -3552,7 +3608,7 @@ {collection} {wsSlug} {focusedItemId} - onStatusChange={handleStatusChange} + onStatusChange={(it, newStatus) => handleStatusChange(it, newStatus, 'status')} onReorder={handleReorder} oncreate={canEditThisCollection ? openQuickCreate : undefined} {itemProgress} diff --git a/web/src/routes/bug3049WebFullBlobWriters.test.ts b/web/src/routes/bug3049WebFullBlobWriters.test.ts index 95110769b..f9d6a883a 100644 --- a/web/src/routes/bug3049WebFullBlobWriters.test.ts +++ b/web/src/routes/bug3049WebFullBlobWriters.test.ts @@ -75,11 +75,18 @@ describe('BUG-3049 — browser item-field writers send a patch, not a blob', () }); it('the collection page status move patches only the group field', () => { + // The patched KEY is what this leg is about, and it is unchanged. The + // VALUE spelling moved under BUG-3057: the lane key is now converted + // through the field's declared type before it is written, because + // assigning the raw key sent a string to a number / checkbox / + // multi_select field and the server refuses those writes. `laneWrite` + // IS `newValue` converted, so "patches only the group field" holds + // exactly as before — only the literal this guard anchors on changed. const body = functionBody( source('./[username]/[workspace]/[collection]/+page.svelte'), 'async function handleStatusChange(', ); - expectPatchOnly(body, 'handleStatusChange', ['[groupField]: newValue']); + expectPatchOnly(body, 'handleStatusChange', ['[fieldKey]: laneWrite.value']); }); it('the conventions status toggle patches only status', () => {