docs(ui): require an explicit gesture to submit an ask, and answer option-less questions by text (RIG-1330) - #1119
Open
rigel-mintaka wants to merge 6 commits into
Open
Conversation
…tion-less questions by text (RIG-1330) Two coupled decisions for the ask surface. Sending an ask becomes an explicit gesture. Today a completing option click issues the ask's one RespondToAsk, and a single-question ask sends on its only click. The prior gate ruled out a per-click respond but never argued for sending without confirmation, and the wire is one-shot: a first toggle on a multi-select question can already ship before the user's second pick. The send tail leaves answerAsk; the submit control becomes the sole send path. An option-less question becomes answerable. compass.v1 allows an empty options list for a free-text question answered by custom_text, but the renderer is a bare For over options with no empty case, so such a question renders an empty container and cannot be answered at all. Design only; no implementation.
|
Merging to
After your PR is submitted to the merge queue, this comment will be automatically updated with its status. If the PR fails, failure details will also be posted here |
|
Compass engineering docs preview: https://compass-ui-rig-1330-ask-free.compass-eng-docs.pages.dev Deployed from Changed pages: |
…e click (RIG-1330) The prior revision read as though an explicit submit meant a pointer trip, and declined "Enter to submit" as a blanket. That conflated two different keys in two different places. Enter while typing in one question's field would ship the whole ask, blanks included, and stays declined. Enter on the focused submit control is the intended primary path: answer every question, tab to submit, press Enter. It comes free from the platform, so the work is to not break it. D5 names the three things that would: a wrapping form, a type="submit" default, and an ancestor keydown handler that cancels Enter. R6 pins those preconditions rather than the keystroke. happy-dom does not implement a button's implicit Enter-to-click default action, measured: a keyDown Enter fires onClick zero times where a click fires once. A test that dispatches Enter and asserts a respond would prove only that the click beside it worked. The keystroke itself is manual verification, and the record now says so rather than implying unit coverage.
…IG-1330) comms.proto's ask messages are a wire form of the agent harness's own ask tool, and cite it by name for the always-available free-text rule. The record never read that tool's design, so it argued from the proto comment alone. Reading it confirms the central ruling and exposes one real gap. Confirms: that tool does not send on a pick either. Its rich dialog is a form the user fills and submits, and even the plain fallback treats a pick as navigation between questions. The gap: it offers "Other (type your own)" on EVERY question, and reserves the label so a caller cannot mint it. Gating the input on options.length === 0 therefore ships a UI narrower than the contract it implements, which the record had framed as a clean boundary the proto drew. It is a scope line and now says so. Widening also inherits a server rule the option-less case never meets: validateQuestionAnswer rejects an option and custom text together on a single-select, so an always-offered Other must make the two exclusive unless allowMultiple. Also recorded: a per-answer note and the "Chat about this" redirect have no field in AskQuestionAnswer at all. Both may already be served by the ask living in a repliable channel, but that is a product call, so the gap is named before anyone reads the two surfaces as at parity.
Matt's directive. The record shipped a strict subset of the contract and deferred the gap to an open question; now it specifies parity. The contract was already there. A merged record derived this proto from the harness tool axis by axis (docs/designs/agent/compass-ask-typed-derivation.md), so every native axis has a wire carrier and the two dropped ones were dropped deliberately. The whole gap is in the UI, which is this record's subject. Free text now renders on every question, not only option-less ones, matching the tool's unconditional Other. That inherits a server rule the option-less case never met: validateQuestionAnswer rejects an option and custom text together on a single-select, and the respond is one-shot, so a UI that lets both stand burns the ask on a refusal. A pick clears the draft and locks the input, which makes type-after-pick impossible rather than merely cleared; allow_multiple keeps both. The send seam enforces it structurally. The presentation axes render too (D6): header as a chip, recommended as a mark that never pre-selects — pre-selecting would stage an answer the user never gave, which the submit control would then ship — and preview capped. recommended's invalid index is ignored, as the tool ignores it. Also corrects a false claim this record made: the tool does NOT 'never send on a pick'. A lone single-select question submits immediately there. That is the one place we diverge, by ruling, and it is now named in Non-goals so it is not restored later as a parity fix.
Review found no hole in the exclusivity design, tracing the same six sequences I did and agreeing the invariant holds. Four smaller findings, all applied. S3 claimed slice T0 but could not compile there: it asserted typed text, and both the field and the recorder that stages it are T1/T2 deliverables. The restage mechanism itself IS T0 and is field-agnostic, so the test splits with it — S3a proves the mechanism on clicks in T0, S3b adds the typed half in T2. An implementer following the old text would have written a test against symbols that do not exist yet. L1 pointed at 'the F5 gap', a finding number from a superseded draft that appears nowhere in this record. Replaced with the description that was already sitting next to it. ChannelView.test.tsx was missing from the per-file enumeration, whose whole job is letting a reviewer tell an intended inversion from a broken test. It cannot pin auto-send (offline store, no comms client, zero send-observing references) but T3 does add DOM under its mounts, so its silence was the ambiguity the section exists to remove. Listed as survives-unchanged. The review also raised path:line citations against rule://planning-evidence. Not applied: it is the convention in every merged record here, so changing this one alone would make it the odd one out. That is a corpus-wide call for Matt, not a unilateral edit.
…RIG-1330) rule://planning-evidence bars path:line in a durable record: the record freezes on merge while the code it cites keeps moving, so a line citation decays into a confident pointer at the wrong code. I had deferred this as a corpus-wide convention call. That was wrong — the rule names a design record as its strongest case, not an exception, and I had already been bitten by exactly this drift on this record two days ago. All 116 citations now name the enclosing symbol. Resolution was by brace-extent containment against the working tree, not indentation: an indent-based resolver named a sibling local that had already closed in 52 of the 116 cases (canSubmit, which ends well above the option buttons it was credited with enclosing). Test citations anchor on the it/test name, which is what survives an edit above it. Two anchors my resolver got wrong, caught by reading the record's own prose: a proto answer-state banner and a store.live.test.ts case, both of which my governing-file heuristic had carried across a paragraph boundary onto store.ts. No technical claim, decision, task, or test id changes. Quotes are untouched — dropping line numbers does not license dropping evidence. Reflow is limited to the 77 paragraphs the citation edits touched, and is word-for-word identical to the unreflowed rewrite. The rest of the corpus has the same problem at scale (8546 instances across 143 of 158 records, two authored after the rule landed). Filed as RIG-3757 rather than bulk-rewriting frozen records unilaterally.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Design record for RIG-1330. Design only — no implementation. Freezes on merge; the impl slices (T0-T3) come after.
Docs-only: 1 file, 876 insertions.
Why this covers two things
RIG-1330 was filed as "close two stub↔
compass.v1RespondToAskgaps". Grounding it found the issue half-stale, and the live half pulled in a second, larger decision.Gap 1 was already shipped. The OQ4 seam widening landed by adding
store.submitAsk(messageId, askId)(wired atChannelView.tsx:133) rather than wideninganswerAsk, whose four-arg signature is unchanged and now only records. Reading the signature alone says the gap is open; it isn't. The issue body has been corrected.Gap 2 is real:
AskBlockis a bare<For each={q().options}>with no empty-case branch, so acompass.v1question with an emptyoptionslist — answerable bycustom_textalone — renders an empty container and cannot be answered at all.Designing that surfaced a sharper problem. A red-team pass found that on a mixed ask (one option question + one free-text question), the completing option click would irrevocably ship whatever half-typed draft was staged at that instant, through a one-shot wire. Asked to choose between keeping that and special-casing it, Matt rejected the framing: an ask should always require an explicit submit.
That ruling is now the record's central decision, and it simplifies the design — no "did text or the click complete it" discrimination, one send path.
What the record decides
answerAsk's send tail leaves;submitAskbecomes the sole send path; the submit control becomes unconditional rather than a skip affordance.isAskCompleteis deleted (its only caller was that tail). The refusal path inverts from rollback to restage, which deletessameAnswerstoo.AskQuestiongainscustomText: string, reversingadapt.ts's deliberate drop. Argued on the drop comment's own criterion ("rather than carried half-rendered" — this design fully renders it), and it inheritspreserveLocalAsks/ restage /sendAskinstead of re-implementing them against a second state shape.answerAskText(...)records locally; both recorders are symmetric, neither sends.customTextper question,""for option questions.isQuestionAnsweredpredicate drives the submit control's enablement and copy.Explicit means confirmed, not clicked
The first revision read as though "explicit submit" meant a pointer trip, and declined "Enter to submit" as a blanket. That conflated two different keys in two different places, and the second commit fixes it.
<button>, so the work is to not break it — D5 names the three things that would: a wrapping<form>, atype="submit"default, and an ancestoronKeyDownthat cancels Enter. None exist today (there is no<form>anywhere inapps/ui/src; the onlyEnterinterception is the composer's, a different component).A claim I made and then falsified. I first wrote that the keyboard path "holds today by construction" and specified a test that presses Enter and asserts one respond. Probing the harness showed that test proves nothing: happy-dom does not implement a button's implicit Enter-to-click default action — a
fireEvent.keyDownEnter firesonClickzero times, where a click fires once. My probe only passed because the assertion ran after a click, which is the near-tautology I reject in review. So R6 now pins the preconditions (BUTTON,type="button", no ancestor<form>, focusable when enabled, Enter left uncancelled — the interceptor probe is observable:defaultPreventedreadstruewith a cancelling ancestor andfalseclean), and the record states plainly that the keystroke itself is manual verification, so a green suite is not misread as proof the key works.Full UI parity with the harness
asktool (Matt's directive)The previous commit found the parity gap and deferred it. Matt ruled: close it. This commit specifies parity.
The contract was already at parity. A merged record derived this proto from the harness tool axis-by-axis —
docs/designs/agent/compass-ask-typed-derivation.md§ "Axis carriers (native → reshaped)". Every native axis has a wire carrier; exactly two (note,chatRedirect) were dropped deliberately, with reasons. So the gap is entirely in the UI, which is this record's subject. The record now defers to that record instead of re-deriving it.Other (type your own)is unconditional.validateQuestionAnswerrejects an option plus custom text on a single-select withErrInvalidArgument, and the respond is one-shot — a UI that lets both stand burns the ask on a refusal. A pick clears the draft and locks the input, so type-after-pick is structurally impossible rather than merely cleared;allowMultiplekeeps both. Enforced again at the send seam, because the cost of needing it once is the whole ask.headeras a chip;recommendedas a visual mark that never pre-selects (pre-selecting stages an answer the user never gave, which the submit control would then ship — a direct collision with D0) and ignores an invalid index as the tool does;previewcapped.New tests pin the rules that can burn a respond: L4/L5 (exclusivity both directions;
allowMultiplecoexistence), R7 (no DOM sequence yields option+text on a single-select), R9 (recommendedmarks, never selects, invalid index doesn't crash).Grounded in the harness
asktool the proto mirrorscomms.proto's ask messages are a wire form of the agent harness's ownasktool, and cite it by name for the always-available free-text rule (comms.proto:426-429). The record had never read that tool's design — it argued from the proto comment alone. Reading it confirms the central ruling and exposes one real gap.Other (type your own)on every question — and reserves the label so a caller cannot mint it. This record gates the input onoptions.length === 0, so it ships a UI narrower than the contract it implements: an option question offers no "Other". The record had framed that as a boundary the proto drew. It isn't; it's a scope line, and it now says so. Open Question 1 was rewritten from a vagueallow_multipledeferral into the actual parity gap, including the rule a widening inherits:validateQuestionAnswerrejects an option and custom text on a single-select (ErrInvalidArgument), so an always-offered "Other" must make the two mutually exclusive unlessallowMultiple.AskQuestionAnswerat all — not droppable-like-header, absent. Both may already be served by an ask living in a channel you can reply to, but that's a product call and a proto change, so the gap is named before anyone reads the two surfaces as at parity.timed_outfield, owned by RIG-1310 and out of scope.Scope change worth reviewing deliberately
D0 changes already-shipped behaviour, not just the new path: today a single-question ask sends on its only click. Six test files reference
answerAsk, andstore.ask-race.test.tscarries 8 assertions on the send observable. The record enumerates every shipped test that inverts, edits, or survives, by file and current assertion — so a reviewer can tell an intended inversion from a broken test. That enumeration is the part most worth checking.Verification
Every citation in the record was checked at source. Two were wrong and are fixed:
messages.go:435-438(claimed: answers recorded in the same write that flipsanswered) actually points atListMessages. The claim is correct —applyAskAnswersetsq.CustomTextthenask.Answered = true, persisted by oneupdateMessageBlocksExecin one tx — so the citation is now anchored on symbol names instead of line numbers.messages.go:404-406(claimed: theErrConflictrefusal) points at unrelated error handling; the real guard is insideapplyAskAnswer. Note this stale range is inherited from existing source comments (store.ts:1690,:1610both cite it), so the record propagated a pre-existing error rather than inventing one.Reading the server also strengthened D3:
validateQuestionAnswerrejects one chosen option plus non-empty custom text on a non-allow_multiplequestion withErrInvalidArgument. So shipping""for option questions is a correctness guard, not tidiness — leaking that case would spend the one-shot respond on a refusal.Gates on the rebased base (
97741c5c):design-ledger-gaterc=0 (318 ledger rows, 134 record headers),moon run root:markdownlintrc=0. Ledger row ids areDL-TBD-Nplaceholders; real ids are allocated when this flipsDECISIONS.md.RIG-1330