diff --git a/AGENTS.md b/AGENTS.md index fd430be3..90ba88b9 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -148,6 +148,16 @@ path does not restore it after a storage error. Treat this as a known durability gap: a proper two-phase refactor must retain or restore the draft until the fenced commit succeeds. +A decision's definition is frozen except for appended options. Any writer may +send `question:option` to add one while the question is open (at most 10 options, +no case-insensitive duplicate label). The server changes the sidecar record, the +open entry, and the plan `Option` projection together under the plan queue, +commits, and only then acknowledges and broadcasts `question:option-added`. The +client's `key` makes a retry return the same option. Shared drafts need no +rewrite: an option has a register only once someone selects it. New free-text +custom answers are not offered; an existing `custom` draft or answer still +renders and derives. + Anchors combine Yjs relative positions with canonical block digests. A position survives surrounding edits; a digest can recover one unique block after a move or epoch replacement. Ambiguous matches must orphan rather than guess. The safe diff --git a/apps/server/src/main.ts b/apps/server/src/main.ts index 48d2d69a..7212ab18 100644 --- a/apps/server/src/main.ts +++ b/apps/server/src/main.ts @@ -322,6 +322,10 @@ async function receive(ws: Socket, raw: string): Promise { if (room.plan) await Questions.cancel(room.plan, server, room.id, ws, frame); return; + case "question:option": + if (room.plan) await Questions.addOption(room.plan, server, room.id, ws, frame); + return; + case "comment:start": if (room.plan) await Comments.start(room.plan, server, room.id, ws, frame); return; diff --git a/apps/server/src/plan/room.ts b/apps/server/src/plan/room.ts index 12929416..c00773f1 100644 --- a/apps/server/src/plan/room.ts +++ b/apps/server/src/plan/room.ts @@ -498,6 +498,40 @@ export function projectAnswer( }); } +/** Append one shared option to a question's projection in the plan. */ +export function appendQuestionOption( + target: Document, + id: string, + question: string, + option: { id: string; label: string; description: string }, +): Mutation | undefined { + return mutate(target, () => { + let found = false; + for (let node of $nodesOfType(QuestionnaireNode)) { + if (node.getId() !== id) continue; + let value = node.getQuestionnaire(); + if (!value.questions.some(item => item.id === question)) continue; + found = true; + node.setQuestionnaire({ + ...value, + questions: value.questions.map(item => + item.id === question + ? { + ...item, + options: [...item.options, { + id: option.id, + label: option.label, + ...(option.description ? { description: option.description } : {}), + }], + } + : item + ), + }); + } + return found; + }); +} + /** Take a questionnaire out of the plan, leaving its record as history. */ export function removeQuestionnaire(target: Document, id: string): Mutation | undefined { return mutate(target, () => { diff --git a/apps/server/src/questions.service.test.ts b/apps/server/src/questions.service.test.ts index 69560b4d..218c9118 100644 --- a/apps/server/src/questions.service.test.ts +++ b/apps/server/src/questions.service.test.ts @@ -286,3 +286,247 @@ test("an active implementation refuses to create a questionnaire", async () => { expect(plan.records.size).toBe(0); expect(room.project(plan.document)).not.toContain("> = []; + let ws = { + data: { handle, client: `client-${handle}`, room: "test" }, + send(raw: string) { + sent.push(JSON.parse(raw)); + }, + } as unknown as Socket; + return { ws, sent }; +} + +function adding( + plan: Plan, + server: Server, + ws: Socket, + id: string, + label: string, + key = "key-0000-0001", +) { + let question = plan.records.get(id)!.definition.questions[0].id; + return Questions.addOption(plan, server, "test", ws, { + kind: "question:option", + ts: 0, + rid: `rid-${key}-${label}`, + id, + question, + key, + label, + }); +} + +test("an appended option is durable in the record, draft store and plan before anyone hears of it", async () => { + let plan = await opened(); + let published: Array<{ kind: string }> = []; + let server = { + publish(_topic: string, raw: string) { + published.push(JSON.parse(raw)); + }, + } as unknown as Server; + let asked = asking(plan, server, definition()); + await asked.created; + let id = [...plan.records.keys()][0]!; + published.length = 0; + let { ws, sent } = member(); + + await adding(plan, server, ws, id, " A third way "); + + let reply = sent.at(-1) as { ok: boolean; option: { id: string; label: string } }; + expect(reply.ok).toBe(true); + expect(reply.option.label).toBe("A third way"); + let item = plan.records.get(id)!.definition.questions[0]; + let options = item.options; + expect(options.map(option => option.label)).toEqual(["Choose this", "A third way"]); + expect(Store.get(plan.questions, id)!.definition.questions[0].options).toEqual(options); + expect(room.project(plan.document)).toContain("A third way"); + // Committed, not merely applied in memory. + expect(plan.persistence.lastSidecar).toContain("A third way"); + expect(published.map(frame => frame.kind)).toEqual(["plan:update", "question:option-added"]); + + // Everyone can choose it and the decision reads as the new label. + let snapshot = Store.snapshot(plan.questions, id); + if (!snapshot.open) throw new Error("not open"); + let model = Question.crdt.Model.fromBinary(new Uint8Array(snapshot.model)) + .fork() as unknown as Question.Model; + model.api.val([item.id, "choice"]).set(reply.option.id); + let edited = Store.edit(plan.questions, id, [...model.api.flush().toBinary()]); + if (!edited.open || !edited.accepted) throw new Error("could not choose the option"); + let claimed = Store.claimSubmit(plan.questions, id, edited.revision, "ana"); + if (!claimed.ok) throw new Error("could not claim"); + expect(claimed.answers).toEqual([{ question: item.question, choices: ["A third way"] }]); + Store.commit(plan.questions, claimed.claim); + await asked.waiting; +}); + +test("repeating an option request with the same key returns the same option", async () => { + let plan = await opened(); + let server = { publish() {} } as unknown as Server; + let asked = asking(plan, server, definition()); + await asked.created; + let id = [...plan.records.keys()][0]!; + let { ws, sent } = member(); + + await adding(plan, server, ws, id, "Once"); + let revision = plan.revision; + await adding(plan, server, ws, id, "Once"); + + let [first, second] = sent as Array<{ ok: boolean; option: { id: string }; repeated?: boolean }>; + expect(second!.option.id).toBe(first!.option.id); + expect(second!.repeated).toBe(true); + expect(plan.records.get(id)!.definition.questions[0].options).toHaveLength(2); + expect(plan.revision).toBe(revision); +}); + +test("an option is refused for duplicates, bounds, settled questions and active implementations", async () => { + let plan = await opened(); + let server = { publish() {} } as unknown as Server; + let asked = asking(plan, server, definition()); + await asked.created; + let id = [...plan.records.keys()][0]!; + let { ws, sent } = member(); + let last = () => sent.at(-1) as { ok: boolean; reason?: string }; + + await adding(plan, server, ws, id, "choose THIS", "key-0000-0002"); + expect(last()).toMatchObject({ ok: false, reason: "duplicate" }); + await adding(plan, server, ws, id, " ", "key-0000-0003"); + expect(last()).toMatchObject({ ok: false, reason: "invalid" }); + await adding(plan, server, ws, id, "ok", "no"); + expect(last()).toMatchObject({ ok: false, reason: "invalid" }); + expect(plan.records.get(id)!.definition.questions[0].options).toHaveLength(1); + + plan.execution = { id: "run-1" } as never; + await adding(plan, server, ws, id, "Blocked", "key-0000-0004"); + expect(last()).toMatchObject({ ok: false, reason: "implementation" }); + plan.execution = undefined; + + for (let index = 0; index < Question.limits.MAX_SHARED_OPTIONS - 1; index++) { + await adding(plan, server, ws, id, `Extra ${index}`, `key-fill-${index}0000`); + expect(last().ok).toBe(true); + } + await adding(plan, server, ws, id, "Overflow", "key-0000-0005"); + expect(last()).toMatchObject({ ok: false, reason: "full" }); + + let claimed = Store.claimCancel(plan.questions, id, "ana"); + if (!claimed.ok) throw new Error("could not claim"); + await adding(plan, server, ws, id, "During", "key-0000-0006"); + expect(last()).toMatchObject({ ok: false, reason: "resolving" }); + Store.commit(plan.questions, claimed.claim); + await adding(plan, server, ws, id, "After", "key-0000-0007"); + expect(last()).toMatchObject({ ok: false, reason: "resolved" }); + await asked.waiting; +}); + +test("a redefined open question survives dump and restore with its older draft", async () => { + let plan = await opened(); + let server = { publish() {} } as unknown as Server; + let asked = asking(plan, server, definition()); + await asked.created; + let id = [...plan.records.keys()][0]!; + let { ws } = member(); + await adding(plan, server, ws, id, "Restored"); + + let restored = Store.restore(JSON.parse(JSON.stringify(Store.dump(plan.questions)))); + let entry = Store.get(restored, id)!; + expect(entry.definition.questions[0].options.map(option => option.label)).toEqual([ + "Choose this", + "Restored", + ]); + expect(Question.read(entry.model, entry.definition)).toBeDefined(); + Store.shutdown(restored); + let claimed = Store.claimCancel(plan.questions, id, "ana"); + if (claimed.ok) Store.commit(plan.questions, claimed.claim); + await asked.waiting; +}); + +test("concurrent appends with one key add one option; with one label add one option", async () => { + let plan = await opened(); + let server = { publish() {} } as unknown as Server; + let asked = asking(plan, server, definition()); + await asked.created; + let id = [...plan.records.keys()][0]!; + let ana = member("ana"); + let bo = member("bo"); + + await Promise.all([ + adding(plan, server, ana.ws, id, "Same key", "key-same-0001"), + adding(plan, server, bo.ws, id, "Same key", "key-same-0001"), + adding(plan, server, ana.ws, id, "Same label", "key-label-0001"), + adding(plan, server, bo.ws, id, "same LABEL", "key-label-0002"), + ]); + + let labels = plan.records.get(id)!.definition.questions[0].options.map(option => option.label); + expect(labels).toEqual(["Choose this", "Same key", "Same label"]); + expect(Object.keys(plan.records.get(id)!.appended!)).toHaveLength(2); + expect((bo.sent.at(-1) as { reason?: string }).reason).toBe("duplicate"); + let claimed = Store.claimCancel(plan.questions, id, "ana"); + if (claimed.ok) Store.commit(plan.questions, claimed.claim); + await asked.waiting; +}); + +test("an implementation claimed while an append waits leaves the room untouched", async () => { + let plan = await opened(); + let server = { publish() {} } as unknown as Server; + let asked = asking(plan, server, definition()); + await asked.created; + let id = [...plan.records.keys()][0]!; + let { ws, sent } = member(); + let before = room.project(plan.document); + + let release = Promise.withResolvers(); + let held = Service.exclusive(plan, () => release.promise); + let pending = adding(plan, server, ws, id, "Late", "key-late-0001"); + plan.claiming = true; + release.resolve(); + await held; + await pending; + plan.claiming = false; + + expect(sent.at(-1)).toMatchObject({ ok: false, reason: "implementation" }); + expect(room.project(plan.document)).toBe(before); + expect(plan.records.get(id)!.definition.questions[0].options).toHaveLength(1); + expect(plan.records.get(id)!.appended).toBeUndefined(); + let claimed = Store.claimCancel(plan.questions, id, "ana"); + if (claimed.ok) Store.commit(plan.questions, claimed.claim); + await asked.waiting; +}); + +test("a failed commit restores the record and the open definition, and nobody is told", async () => { + let plan = await opened(); + let published: Array<{ kind: string }> = []; + let server = { + publish(_topic: string, raw: string) { + published.push(JSON.parse(raw)); + }, + } as unknown as Server; + let asked = asking(plan, server, definition()); + await asked.created; + let id = [...plan.records.keys()][0]!; + let { ws, sent } = member(); + published.length = 0; + + let original = plan.persistence.storage.collaboration.commit; + let fatal = plan.persistence.fatal; + plan.persistence.fatal = () => {}; + plan.persistence.storage.collaboration.commit = () => Promise.reject(new Error("disk full")); + let quiet = console.error; + console.error = () => {}; + try { + await adding(plan, server, ws, id, "Lost", "key-lost-0001"); + } finally { + console.error = quiet; + plan.persistence.storage.collaboration.commit = original; + plan.persistence.fatal = fatal; + } + + expect(sent.at(-1)).toMatchObject({ kind: "session:error" }); + expect(published).toEqual([]); + expect(plan.records.get(id)!.definition.questions[0].options).toHaveLength(1); + expect(plan.records.get(id)!.appended).toBeUndefined(); + expect(Store.get(plan.questions, id)!.definition.questions[0].options).toHaveLength(1); + let claimed = Store.claimCancel(plan.questions, id, "ana"); + if (claimed.ok) Store.commit(plan.questions, claimed.claim); + await asked.waiting; +}); diff --git a/apps/server/src/questions/service.ts b/apps/server/src/questions/service.ts index 9b5e9ed9..f007c2ce 100644 --- a/apps/server/src/questions/service.ts +++ b/apps/server/src/questions/service.ts @@ -75,6 +75,11 @@ export type Record = { at?: number; /** Where in the prose each of its decisions lives. */ anchors?: Wired.WidgetAnchors; + /** + * Option-append idempotency keys to the option each created. Bounded by the + * option limit, and durable so a retry after a restart still finds it. + */ + appended?: { [key: string]: string }; }; function decide( @@ -336,6 +341,175 @@ export async function submit( }); } +const ADD_OPTION_REFUSAL: { [reason in "resolved" | "resolving"]: string } = { + resolved: "This question has already been decided", + resolving: "This question is being decided", +}; + +/** + * Append an option to an open question, for everyone. + * + * Ordering follows the rest of this module: the record, the open entry and the + * plan projection change together under the plan's exclusive queue, the fenced + * commit happens, and only then does anyone hear about it. The shared draft is + * not touched, so nothing another member has already chosen can change. + */ +export async function addOption( + plan: Plan, + server: Server, + roomId: string, + ws: Socket, + msg: Request, +): Promise { + let refuse = (reason: Wire.AddOption.Refusal, message: string) => + reply(ws, msg.rid, { + kind: "question:option", + ts: 0, + id: msg.id, + ok: false, + reason, + message, + }); + if (Service.implementationActive(plan)) { + return refuse("implementation", "An implementation is running; decisions cannot change"); + } + + let outcome: Wire.AddOption.Reply | undefined; + let added: Wire.OptionAdded | undefined; + await Service.exclusive(plan, async () => { + // An implementation may have claimed the plan while this waited in the + // queue. Refuse before touching the live document: `publish` would throw + // after the Yjs mutation, leaving the room ahead of its durable state. + if (Service.implementationActive(plan)) { + outcome = { + kind: "question:option", + ts: 0, + id: msg.id, + ok: false, + reason: "implementation", + message: "An implementation is running; decisions cannot change", + }; + return; + } + let record = plan.records.get(msg.id); + let entry = Store.get(plan.questions, msg.id); + if (!record || !entry || record.status !== "open") { + outcome = { + kind: "question:option", + ts: 0, + id: msg.id, + ok: false, + reason: "resolved", + message: ADD_OPTION_REFUSAL.resolved, + }; + return; + } + let applied = typeof msg.key === "string" && Object.hasOwn(record.appended ?? {}, msg.key) + ? record.appended![msg.key] + : undefined; + let existing = applied + ? entry.definition.questions[0].options.find(option => option.id === applied) + : undefined; + if (existing) { + outcome = { + kind: "question:option", + ts: 0, + id: msg.id, + ok: true, + option: existing, + definition: entry.definition, + repeated: true, + }; + return; + } + if (entry.claim) { + outcome = { + kind: "question:option", + ts: 0, + id: msg.id, + ok: false, + reason: "resolving", + message: ADD_OPTION_REFUSAL.resolving, + }; + return; + } + + let result = Question.appendOption(entry.definition, { + question: msg.question, + key: msg.key, + label: msg.label, + ...(msg.description === undefined ? {} : { description: msg.description }), + }, ulid()); + if (!result.ok) { + outcome = { + kind: "question:option", + ts: 0, + id: msg.id, + ok: false, + reason: result.reason, + message: result.message, + }; + return; + } + + let previous = { record, definition: entry.definition }; + let mutation: room.Mutation | undefined; + try { + mutation = room.appendQuestionOption(plan.document, msg.id, msg.question, result.option); + } catch (err) { + console.error("[questions] could not add the option to the plan:", err); + outcome = { + kind: "question:option", + ts: 0, + id: msg.id, + ok: false, + reason: "invalid", + message: "Could not add the option", + }; + return; + } + plan.records.set(msg.id, { + ...record, + definition: result.definition, + appended: { ...record.appended, [msg.key]: result.option.id }, + }); + Store.redefine(plan.questions, msg.id, result.definition); + try { + if (mutation) await Service.publish(plan, server, roomId, mutation); + else await Service.persistExclusive(plan); + } catch (err) { + plan.records.set(msg.id, previous.record); + Store.restoreDefinition(plan.questions, msg.id, previous.definition); + throw err; + } + outcome = { + kind: "question:option", + ts: 0, + id: msg.id, + ok: true, + option: result.option, + definition: result.definition, + }; + added = { + kind: "question:option-added", + ts: 0, + id: msg.id, + question: msg.question, + option: result.option, + definition: result.definition, + by: ws.data.handle, + }; + }).catch(err => { + console.error("[questions] could not save the option:", err); + outcome = undefined; + added = undefined; + }); + + if (!outcome) return fail(ws, msg.rid, "could not save the option"); + reply(ws, msg.rid, outcome); + if (added) broadcast(server, roomId, added); +} + /** * Decline to answer. * diff --git a/apps/server/src/questions/store.ts b/apps/server/src/questions/store.ts index 33cb8cea..32069839 100644 --- a/apps/server/src/questions/store.ts +++ b/apps/server/src/questions/store.ts @@ -163,6 +163,35 @@ export function get(questions: Questions, id: string): Open | undefined { return questions.open.get(id); } +/** + * Replace an open question's definition with one that only appended options. + * + * Returns what to restore if the durable half fails, or why it is refused. The + * draft is untouched: its keys are created lazily, so existing drafts stay valid. + */ +export function redefine( + questions: Questions, + id: string, + definition: DecisionDefinition, +): { ok: true; previous: DecisionDefinition } | { ok: false; reason: "resolved" | "resolving" } { + let entry = questions.open.get(id); + if (!entry) return { ok: false, reason: "resolved" }; + if (entry.claim) return { ok: false, reason: "resolving" }; + let previous = entry.definition; + entry.definition = definition; + return { ok: true, previous }; +} + +/** Undo `redefine` after its durable half failed. */ +export function restoreDefinition( + questions: Questions, + id: string, + definition: DecisionDefinition, +): void { + let entry = questions.open.get(id); + if (entry) entry.definition = definition; +} + /** Everything still open, for a client that has just joined. */ export function outstanding( questions: Questions, diff --git a/apps/web/src/theme.css b/apps/web/src/theme.css index 65dcd258..21919980 100644 --- a/apps/web/src/theme.css +++ b/apps/web/src/theme.css @@ -627,7 +627,7 @@ body { padding-block: 0.5rem; } -.question-custom-answer { +.question-field { scroll-margin-block-end: calc(var(--keyboard-inset, 0px) + 1rem); } @@ -821,6 +821,33 @@ label.question-option:has(.question-input:not(:disabled)) { color: var(--color-text-secondary); } +/* The add row is a real button, not a label around a hidden input. */ +button.question-add { + inline-size: 100%; + border: 0; + background: transparent; + text-align: start; + cursor: pointer; +} + +button.question-add:not(:disabled):hover { + background: var(--color-hover); +} + +button.question-add:focus-visible { + outline: var(--focus-ring-width) solid var(--focus-ring-color); + outline-offset: var(--focus-ring-offset); +} + +button.question-add:disabled { + cursor: default; +} + +/* The new option, shown before the server confirms it. */ +.question-pending { + opacity: 0.6; +} + .question-adding { outline: var(--edge-width) solid var(--color-control-edge); outline-offset: calc(-1 * var(--edge-width)); diff --git a/e2e/responsive-decisions.e2e.ts b/e2e/responsive-decisions.e2e.ts index fe305aa2..caae2ba6 100644 --- a/e2e/responsive-decisions.e2e.ts +++ b/e2e/responsive-decisions.e2e.ts @@ -89,7 +89,7 @@ for (let viewport of [{ width: 320, height: 568 }, { width: 390, height: 844 }]) await page.goto(`/channels/${room}`); let card = questionnaire(page).filter({ hasText: LONG_QUESTIONS[0]!.header }); await expect(card).toBeVisible(); - await expect(card.getByRole("textbox", { name: "Add an option" })).toHaveCount(0); + await expect(card.getByRole("textbox", { name: "New option" })).toHaveCount(0); await expectNoHorizontalOverflow(page); let firstChoice = card.getByRole("radio", { name: "Use the compact layout" }); @@ -132,10 +132,10 @@ for (let viewport of [{ width: 320, height: 568 }, { width: 390, height: 844 }]) .getByRole("button", { name: /Decisions, 8 unanswered/ }), ).toBeVisible(); - let customChoice = card.getByRole("radio", { name: "Add an option" }); - await customChoice.focus(); - await page.keyboard.press("Space"); - let custom = card.getByRole("textbox", { name: "Add an option" }); + let addRow = card.getByRole("button", { name: "Add an option" }); + await addRow.focus(); + await page.keyboard.press("Enter"); + let custom = card.getByRole("textbox", { name: "New option" }); await expect(custom).toBeFocused(); await setVisualViewport(page, { event: "resize", height: 360 }); await expect.poll(() => diff --git a/e2e/sidecar.e2e.ts b/e2e/sidecar.e2e.ts index b79249d5..eb47d473 100644 --- a/e2e/sidecar.e2e.ts +++ b/e2e/sidecar.e2e.ts @@ -560,8 +560,8 @@ test("decision cards save independently with progressive custom answers", async }); let saveStorage = storage.getByRole("button", { name: "Save", exact: true }); - await expect(storage.getByRole("textbox", { name: "Add an option" })).toHaveCount(0); - await expect(scope.getByRole("textbox", { name: "Add an option" })).toHaveCount(0); + await expect(storage.getByRole("textbox", { name: "New option" })).toHaveCount(0); + await expect(scope.getByRole("textbox", { name: "New option" })).toHaveCount(0); await storage.getByRole("radio", { name: /On disk as MDX/ }).check(); await saveStorage.click(); @@ -643,26 +643,32 @@ test("decision cards save independently with progressive custom answers", async await expect(history).not.toHaveAttribute("aria-controls"); await expect(historyContent).toHaveCount(0); - let customChoice = scope.getByRole("checkbox", { name: "Add an option" }); - await customChoice.focus(); - await page.keyboard.press("Space"); - let custom = scope.getByRole("textbox", { name: "Add an option" }); - await expect(custom).toBeFocused(); + let addRow = scope.getByRole("button", { name: "Add an option" }); + await addRow.focus(); + await page.keyboard.press("Enter"); + let field = scope.getByRole("textbox", { name: "New option" }); + await expect(field).toBeFocused(); let saveScope = scope.getByRole("button", { name: "Save", exact: true }); await expect(saveScope).toBeDisabled(); - await custom.press("Escape"); - await expect(custom).toHaveCount(0); - await expect(customChoice).toBeFocused(); - await customChoice.press("Space"); - await expect(custom).toBeFocused(); - await custom.fill("Only collaborative anchors"); - await scope.getByRole("checkbox", { name: "Anchors" }).check(); - await expect(custom).toHaveCount(0); - await customChoice.click(); - custom = scope.getByRole("textbox", { name: "Add an option" }); - await expect(custom).toHaveValue("Only collaborative anchors"); - await expect(custom).toBeFocused(); - await scope.getByRole("button", { name: "Save", exact: true }).click(); + await field.press("Escape"); + await expect(field).toHaveCount(0); + await expect(addRow).toBeFocused(); + await addRow.click(); + field = scope.getByRole("textbox", { name: "New option" }); + await field.fill("Only collaborative anchors"); + await field.press("Enter"); + // It joins everyone's list as the next lettered row and is not chosen for anyone. + let added = scope.getByRole("checkbox", { name: "Only collaborative anchors" }); + await expect(added).toBeVisible(); + await expect(added).not.toBeChecked(); + await expect(scope.locator("label", { hasText: "Only collaborative anchors" })).toContainText( + "D", + ); + await expect(addRow).toBeFocused(); + await expect(saveScope).toBeDisabled(); + await added.check(); + await scope.getByRole("checkbox", { name: /^Anchors/ }).check(); + await saveScope.click(); await expect(questionnaire(page).filter({ hasText: "Which of these belong in the first cut?" })) .toContainText("Only collaborative anchors"); }); @@ -744,6 +750,73 @@ test("a rejected save is announced as an alert with motion feedback", async ({ j await expect(card).not.toContainText("Answered by"); }); +test("an option one member adds is shared, durable, and choosable by another", async ({ join, seed }) => { + await seed(PROSE); + let ana = await join("ana"); + let bo = await join("bo"); + let storage = (page: Page) => + questionnaire(page).filter({ + has: page.getByRole("heading", { name: "Where should room state live?" }), + }); + for (let page of [ana, bo]) await page.getByRole("button", { name: /^Decisions/ }).click(); + await expect(storage(bo).getByRole("radio")).toHaveCount(2); + + await storage(ana).getByRole("button", { name: "Add an option" }).click(); + let field = storage(ana).getByRole("textbox", { name: "New option" }); + await expect(field).toBeFocused(); + await field.fill("In PostgreSQL"); + await field.press("Enter"); + + // Everyone sees it as the next lettered row, chosen by nobody. + for (let page of [ana, bo]) { + let row = storage(page).getByRole("radio", { name: "In PostgreSQL" }); + await expect(row).toBeVisible(); + await expect(row).not.toBeChecked(); + await expect(storage(page).locator("label", { hasText: "In PostgreSQL" })).toContainText("C"); + } + await expect(storage(ana).getByRole("textbox", { name: "New option" })).toHaveCount(0); + + // The plan carries it too, which is what the Planner reads. + await ana.getByRole("button", { name: "Document", exact: true }).click(); + await expect(ana.locator(`[data-document-view="plan"]`).getByText("In PostgreSQL")) + .toBeVisible(); + await ana.getByRole("button", { name: /^Decisions/ }).click(); + + // It survives a reload, and the second member can choose and save it. + await bo.reload(); + await bo.getByRole("button", { name: /^Decisions/ }).click(); + let chosen = storage(bo).getByRole("radio", { name: "In PostgreSQL" }); + await expect(chosen).toBeVisible(); + await chosen.check(); + await storage(bo).getByRole("button", { name: "Save", exact: true }).click(); + + for (let page of [ana, bo]) { + await page.getByRole("button", { name: "1 resolved" }).click(); + await expect(questionnaire(page).filter({ hasText: "Where should room state live?" })) + .toContainText("In PostgreSQL"); + } +}); + +test("adding a duplicate option is refused and keeps what was typed", async ({ join, seed }) => { + await seed(PROSE); + let page = await join("ana"); + await page.getByRole("button", { name: /^Decisions/ }).click(); + let card = questionnaire(page).filter({ + has: page.getByRole("heading", { name: "Where should room state live?" }), + }); + + await card.getByRole("button", { name: "Add an option" }).click(); + let field = card.getByRole("textbox", { name: "New option" }); + await field.fill("in sqlite"); + await field.press("Enter"); + + let alert = card.getByRole("alert"); + await expect(alert).toContainText("Couldn’t add option"); + await expect(alert).toContainText("already exists"); + await expect(field).toHaveValue("in sqlite"); + await expect(card.getByRole("radio")).toHaveCount(2); +}); + test("discarding asks first", async ({ join, seed }) => { await seed(PROSE); let page = await join("ana"); diff --git a/packages/dialect/src/nodes/questionnaire.ts b/packages/dialect/src/nodes/questionnaire.ts index 677c3079..814b6e6e 100644 --- a/packages/dialect/src/nodes/questionnaire.ts +++ b/packages/dialect/src/nodes/questionnaire.ts @@ -2,9 +2,9 @@ * Durable questionnaires. * * Unlike the other containers this is atomic. A questionnaire's definition is - * immutable once created, and its answer is owned by the sidecar record rather - * than by the document, so there is nothing inside it for two people to edit - * concurrently. Modelling it as a decorator keeps it selectable, movable and + * fixed once created apart from options the server appends while it is open, + * and its answer is owned by the sidecar record rather than by the document, so + * there is nothing inside it for two people to edit concurrently. Modelling it as a decorator keeps it selectable, movable and * deletable as one unit while making its contents unwritable by construction. * * The `` written into source is a projection for readability. The diff --git a/packages/editor/src/widgets/questionnaire.tsx b/packages/editor/src/widgets/questionnaire.tsx index 5b53e49a..97473c3f 100644 --- a/packages/editor/src/widgets/questionnaire.tsx +++ b/packages/editor/src/widgets/questionnaire.tsx @@ -1,8 +1,8 @@ /** * A questionnaire, as the decisions pane shows it. * - * The definition is immutable and the answer is owned by the server's record, - * so this never writes to the document — an agent rewriting the plan cannot + * The definition only ever grows by appended options and the answer is owned by + * the server's record, so this never writes to the document — an agent rewriting the plan cannot * overwrite a decision. What the plan node carries is a projection, kept so * the source reads correctly on its own. */ @@ -187,6 +187,7 @@ function Undecided( drafts={state.drafts} error={state.error} errorClassName="editor-motion-feedback" + onAddOption={editable ? state.addOption : undefined} onCancel={editable ? state.cancel : undefined} onChange={editable ? state.change : undefined} onSubmit={editable ? state.submit : undefined} diff --git a/packages/protocol/question.d.ts b/packages/protocol/question.d.ts index 753d6a3d..e22e5c9f 100644 --- a/packages/protocol/question.d.ts +++ b/packages/protocol/question.d.ts @@ -5,7 +5,9 @@ type KIND = Frame & { kind: K }; /** * Collaborative questions. * - * A questionnaire is immutable once asked. Its answer is not: it lives in a + * A questionnaire's definition is frozen once asked, with one exception: any + * member with write access may append an option while it is open (`AddOption`). + * Its answer is not frozen either: it lives in a * shared CRDT owned by the server, so everyone present converges on one draft * before somebody submits it back to the agent that is waiting on it. * @@ -19,6 +21,7 @@ export declare namespace Question { | Request | Request | Request + | Request | Presence.Input; export type Outgoing = @@ -28,6 +31,8 @@ export declare namespace Question { | Edit.Reply | Submit.Reply | Cancel.Reply + | AddOption.Reply + | OptionAdded | Presence.Output | Resolved; @@ -203,6 +208,66 @@ export declare namespace Question { ); } + export namespace AddOption { + /** + * Append one option to an open question, for everyone. + * + * `key` is an idempotency token chosen by the client: repeating a request + * with the same key returns the option already added instead of adding + * another. The server mints the option's identity. + */ + export type Ask = KIND<"question:option"> & { + id: string; + question: string; + key: string; + label: string; + description?: string; + }; + + export type Refusal = + /** Malformed or out-of-bounds input. */ + | "invalid" + /** An option with that label already exists. */ + | "duplicate" + /** The question has reached its option limit. */ + | "full" + /** The questionnaire is settled, or no longer exists. */ + | "resolved" + /** A submit or cancel is already in flight. */ + | "resolving" + /** An implementation run forbids plan and decision changes. */ + | "implementation"; + + export type Reply = + & KIND<"question:option"> + & { id: string } + & ( + | { + ok: true; + option: Option; + /** The whole definition as it now stands. */ + definition: DecisionDefinition; + /** True when this key had already been applied. */ + repeated?: boolean; + } + | { ok: false; reason: Refusal; message: string } + ); + } + + /** + * An option was appended, after it became durable. + * + * Carries the complete definition rather than a delta, so a duplicate or + * late delivery is harmless. + */ + export type OptionAdded = KIND<"question:option-added"> & { + id: string; + question: string; + option: Option; + definition: DecisionDefinition; + by: string; + }; + /** The questionnaire is closed. Nobody may answer it further. */ export type Resolved = KIND<"question:resolved"> & { id: string; diff --git a/packages/question/src/draft.ts b/packages/question/src/draft.ts index 3d26556c..fa0036a2 100644 --- a/packages/question/src/draft.ts +++ b/packages/question/src/draft.ts @@ -124,11 +124,22 @@ export function read(model: Model, definition: Definition): Drafts { let optionNode = node.get("options"); if (!(optionNode instanceof crdt.ObjNode)) reject(`${question.id}.options must be an object`); - keys(optionNode, question.options.map(option => option.id), `${question.id}.options`); + // Options can be appended after a draft exists, and a draft never has to + // be rewritten for it: a key is present only once somebody selects that + // option. Keys must still belong to the definition. + let known = new Set(question.options.map(option => option.id)); + for (let key of optionNode.keys.keys()) { + if (!known.has(key)) reject(`${question.id}.options has invalid keys`); + } let options: Record = {}; for (let option of question.options) { - let selected = register(optionNode.get(option.id), `${question.id}.options.${option.id}`); + let node = optionNode.get(option.id); + if (node === undefined) { + options[option.id] = false; + continue; + } + let selected = register(node, `${question.id}.options.${option.id}`); if (typeof selected !== "boolean") { reject(`${question.id}.options.${option.id} must be a boolean LWW register`); } diff --git a/packages/question/src/index.ts b/packages/question/src/index.ts index 3ca45f27..63ff6312 100644 --- a/packages/question/src/index.ts +++ b/packages/question/src/index.ts @@ -12,8 +12,8 @@ export * as limits from "./limits"; -export { assertCallId, decision, normalize, QuestionError, reject } from "./schema"; -export type { Answer, DecisionDefinition, Definition, Item, Option } from "./schema"; +export { appendOption, assertCallId, decision, normalize, QuestionError, reject } from "./schema"; +export type { Answer, Appended, DecisionDefinition, Definition, Item, Option } from "./schema"; export { answered, apply, assertPatch, create, read, restore } from "./draft"; export type { Applied, Draft, Drafts, Mode, Model } from "./draft"; diff --git a/packages/question/src/limits.ts b/packages/question/src/limits.ts index 7d622d0c..7a3fb939 100644 --- a/packages/question/src/limits.ts +++ b/packages/question/src/limits.ts @@ -7,12 +7,17 @@ export const MAX_QUESTIONS = 10; export const MAX_OPTIONS = 20; +/** The most options a question may hold once members start appending their own. */ +export const MAX_SHARED_OPTIONS = 10; export const MAX_HEADER = 80; export const MAX_QUESTION = 1_000; export const MAX_LABEL = 200; export const MAX_DESCRIPTION = 1_000; export const MAX_CUSTOM = 4_000; +/** Longest idempotency key for appending an option. */ +export const MAX_KEY = 64; + /** One collaborative edit to the shared draft. */ export const MAX_PATCH_BYTES = 64 * 1024; diff --git a/packages/question/src/question.test.ts b/packages/question/src/question.test.ts index 2d3c929e..9c6d9b97 100644 --- a/packages/question/src/question.test.ts +++ b/packages/question/src/question.test.ts @@ -1,9 +1,9 @@ import { describe, expect, it } from "bun:test"; import { derive, incomplete, summarize } from "./answer"; -import { answered, assertPatch, create, read } from "./draft"; +import { answered, apply, assertPatch, crdt, create, read } from "./draft"; import * as limits from "./limits"; -import { normalize, QuestionError } from "./schema"; +import { appendOption, normalize, QuestionError } from "./schema"; import type { Definition } from "./schema"; @@ -84,10 +84,14 @@ describe("draft", () => { header: "Other", question: "?", multiple: false, - options: [{ label: "a", description: "" }], + options: [{ label: "a", description: "" }, { label: "b", description: "" }, { + label: "c", + description: "", + }], }], }); - // A draft built for one definition must not validate against another. + // A draft built for one definition must not validate against another: + // it may lack keys (options appended later) but never hold unknown ones. expect(() => read(create(other), definition)).toThrow(QuestionError); }); @@ -178,3 +182,86 @@ describe("derive", () => { expect(summarize({ question: "?", custom: "Something else" })).toBe("Something else"); }); }); + +const KEY = "0123456789abcdef"; + +describe("appendOption", () => { + let definition = normalize(tool()); + let question = definition.questions[0]!.id; + let input = (label: unknown, extra: Record = {}) => ({ + question, + key: KEY, + label, + ...extra, + }); + + it("returns a new frozen definition with the option last", () => { + let single = { questions: [definition.questions[0]!] as [(typeof definition.questions)[0]] }; + let result = appendOption(single, input(" Shadow ", { description: " dark " }), "NEW"); + expect(result.ok).toBe(true); + if (!result.ok) return; + expect(result.option).toEqual({ id: "NEW", label: "Shadow", description: "dark" }); + expect(result.definition.questions[0].options.map(option => option.id)).toEqual([ + "o0", + "o1", + "NEW", + ]); + expect(Object.isFrozen(result.definition.questions[0].options)).toBe(true); + // The original is untouched. + expect(single.questions[0].options).toHaveLength(2); + }); + + let single = { questions: [definition.questions[0]!] as [(typeof definition.questions)[0]] }; + + it("rejects empty, oversized, or non-text labels", () => { + for (let label of [" ", "x".repeat(limits.MAX_LABEL + 1), 4, undefined]) { + let result = appendOption(single, input(label), "NEW"); + expect(result.ok ? "ok" : result.reason).toBe("invalid"); + } + }); + + it("rejects a case-insensitive duplicate", () => { + let result = appendOption(single, input(" canary "), "NEW"); + expect(result.ok ? "ok" : result.reason).toBe("duplicate"); + }); + + it("rejects an unknown question and a malformed key", () => { + expect(appendOption(single, { ...input("x"), question: "nope" }, "NEW").ok).toBe(false); + expect(appendOption(single, { ...input("x"), key: "short" }, "NEW").ok).toBe(false); + }); + + it("stops at the option limit", () => { + let current = single; + for (let index = 0; index < limits.MAX_SHARED_OPTIONS - 2; index++) { + let result = appendOption(current, input(`Extra ${index}`), `ID${index}`); + expect(result.ok).toBe(true); + if (result.ok) current = result.definition; + } + let result = appendOption(current, input("One more"), "LAST"); + expect(result.ok ? "ok" : result.reason).toBe("full"); + }); + + it("keeps an existing draft valid and lets the new option be chosen", () => { + let model = create(single); + let result = appendOption(single, input("Shadow"), "NEW"); + if (!result.ok) throw new Error("append failed"); + + // The old draft reads against the larger definition; the new option is unselected. + let drafts = read(model, result.definition); + expect(drafts[question]!.options).toEqual({ o0: false, o1: false, NEW: false }); + + // A client selects it by creating its key. + let fork = model.fork(); + fork.api.obj([question, "options"]).set({ + NEW: crdt.schema.val(crdt.schema.con(true)), + }); + let patch = fork.api.flush(); + let applied = apply(model, result.definition, [...patch.toBinary()]); + expect(applied.ok).toBe(true); + if (!applied.ok) return; + expect(read(applied.model, result.definition)[question]!.options.NEW).toBe(true); + + // Against the old definition the same patch is an unknown key. + expect(apply(model, single, [...patch.toBinary()]).ok).toBe(false); + }); +}); diff --git a/packages/question/src/react/index.ts b/packages/question/src/react/index.ts index 57b7351d..4fbc7a01 100644 --- a/packages/question/src/react/index.ts +++ b/packages/question/src/react/index.ts @@ -6,6 +6,11 @@ */ export { QuestionView } from "./question-view"; -export type { Collaborator, QuestionStepRenderProps, QuestionViewProps } from "./question-view"; +export type { + AddOptionResult, + Collaborator, + QuestionStepRenderProps, + QuestionViewProps, +} from "./question-view"; export { forget, useQuestionnaire } from "./use-questionnaire"; export type { QuestionnaireOptions, QuestionnaireState, Transport } from "./use-questionnaire"; diff --git a/packages/question/src/react/question-view.test.ts b/packages/question/src/react/question-view.test.ts index 77543582..0587907f 100644 --- a/packages/question/src/react/question-view.test.ts +++ b/packages/question/src/react/question-view.test.ts @@ -106,15 +106,39 @@ test("options carry letter tiles and the last row offers to add one", () => { expect(markup).not.toContain("Write a custom answer"); }); -test("an existing custom answer opens the add row as a field with the next letter", () => { +test("an existing custom answer still renders, as a selected row before the add row", () => { let markup = renderToStaticMarkup(createElement(QuestionView, { definition: { questions: [ROLLOUT] }, drafts: { rollout: { mode: "custom", choice: "", options: {}, custom: "Opt-in beta" } }, + onAddOption: async () => ({ ok: true as const }), })); - expect(markup).toContain("C<"); + expect(markup.indexOf("Opt-in beta")).toBeLessThan(markup.lastIndexOf("Add an option")); + expect(markup).toContain('checked=""'); +}); + +test("the add row is disabled without a handler and hidden at the option limit", () => { + let view = (options: typeof ROLLOUT.options, onAddOption?: () => Promise<{ ok: true }>) => + renderToStaticMarkup(createElement(QuestionView, { + definition: { questions: [{ ...ROLLOUT, options }] }, + drafts: {}, + onAddOption, + })); + + expect(view(ROLLOUT.options)).toMatch(/question-add"[^>]*disabled/); + expect(view(ROLLOUT.options, async () => ({ ok: true }))).not.toMatch( + /question-add"[^>]*disabled/, + ); + + let full = Array.from({ length: 10 }, (_, index) => ({ + id: `o${index}`, + label: `Option ${index}`, + description: "", + })); + expect(view(full, async () => ({ ok: true }))).not.toContain("Add an option"); }); test("a multiple-choice question says so once, under its title", () => { diff --git a/packages/question/src/react/question-view.tsx b/packages/question/src/react/question-view.tsx index d6af805c..40cb0602 100644 --- a/packages/question/src/react/question-view.tsx +++ b/packages/question/src/react/question-view.tsx @@ -12,6 +12,7 @@ import { useEffect, useId, useRef, useState } from "react"; import { CheckIcon, ChevronIcon, DecisionIcon, PlusIcon, WarningIcon } from "@chopin/icons"; +import { MAX_LABEL, MAX_SHARED_OPTIONS } from "../limits"; import { answered } from "../draft"; import type { ReactNode } from "react"; @@ -25,6 +26,8 @@ export type Collaborator = { question?: string; }; +export type AddOptionResult = { ok: true } | { ok: false; message: string }; + export type QuestionStepRenderProps = { children: ReactNode; question: string; @@ -37,6 +40,11 @@ export type QuestionViewProps = { onChange?: (question: string, change: Partial) => void; onSubmit?: () => void; onCancel?: () => void; + /** + * Append an option for everyone. Absent where the viewer may not write; the + * row is then shown disabled. Resolves once the server has made it durable. + */ + onAddOption?: (question: string, label: string) => Promise; disabled?: boolean; submitting?: boolean; status?: "open" | "answered" | "cancelled"; @@ -166,28 +174,64 @@ function Choices( ); } -/** The last row: a prompt to add an option, which becomes the field for it. */ -function Custom( - { question, draft, disabled, name, onChange }: { +/** + * An open draft that was already in free-text mode before options became + * shared. Shown as the selected row it was; choosing any option leaves it, and + * nothing new can enter this mode. + */ +function LegacyCustom( + { question, draft, name }: { question: Item; draft: Draft; name: string }, +) { + return ( + + ); +} + +/** + * The last row: a prompt to add an option, which becomes the field for it. + * + * Enter adds it for everyone, Escape cancels. The new row is shown straight + * away, dimmed, until the server confirms it; a rejection reopens the field + * with the text intact. + */ +function AddOption( + { question, offset, disabled, onAdd, onFailed }: { question: Item; - draft: Draft | undefined; + /** Rows already shown below the options, such as a legacy custom answer. */ + offset: number; disabled: boolean; - name: string; - onChange?: (change: Partial) => void; + onAdd?: (label: string) => Promise; + onFailed: (message: string | undefined) => void; }, ) { - let active = draft?.mode === "custom"; - let textarea = useRef(null); - let row = useRef(null); - let focusOnReveal = useRef(false); - let focusOnClose = useRef(false); + let [text, setText] = useState(null); + let [pending, setPending] = useState(null); + let input = useRef(null); + let trigger = useRef(null); + let focus = useRef<"field" | "trigger">(undefined); + let letterIndex = question.options.length + offset; useEffect(() => { - if (active && focusOnReveal.current) textarea.current?.focus(); - else if (!active && focusOnClose.current) row.current?.focus(); - focusOnReveal.current = false; - focusOnClose.current = false; - }, [active]); + let target = focus.current; + focus.current = undefined; + if (target === "field") input.current?.focus(); + else if (target === "trigger") trigger.current?.focus(); + }); useEffect(() => { let viewport = window.visualViewport; @@ -196,7 +240,7 @@ function Custom( let reveal = () => { let previous = height; height = viewport.height; - let control = textarea.current; + let control = input.current; if (height >= previous || document.activeElement !== control || !control) return; let bounds = control.getBoundingClientRect(); let top = viewport.offsetTop; @@ -209,53 +253,99 @@ function Custom( return () => viewport.removeEventListener("resize", reveal); }, []); - if (!active) { + let add = async () => { + let label = text?.trim(); + if (!label || !onAdd || pending !== null) return; + setPending(label); + onFailed(undefined); + let result: AddOptionResult; + try { + result = await onAdd(label); + } catch { + result = { ok: false, message: "Could not add this option." }; + } + setPending(null); + if (result.ok) { + setText(null); + focus.current = "trigger"; + } else { + onFailed(result.message); + focus.current = "field"; + } + }; + + // Until the server confirms, the new row stands where the field was. If the + // broadcast beat the acknowledgement, the real row is already listed above. + if (pending !== null) { + let known = question.options.some(option => + option.label.trim().toLowerCase() === pending.toLowerCase() + ); + if (known) return null; return ( -