githubnext/chopinPublic

Let's compose rich plans together

Decision card: inline duplicate warning and calmer presence #347

MergedMaggieAppletonmerged 6 commits intomainfromrefs/pull/347/head
  1. MA

    ## Why

    Adding a duplicate option showed a large alert that shifted the decision card. Presence included the viewer's own avatar and moved the header actions when another person arrived.

    ## What changed

    - A duplicate label now shows an inline, case-insensitive warning while typing. The reserved line keeps the card steady; Enter sends no duplicate request. The server error remains the fallback for a race. - The warning uses the shared danger text and surface tokens. Adding a valid option does not alter anyone's shared selection. - Decision cards omit the viewer from presence. Other faces appear before the header actions and fade in when reduced motion is off.

    ## Screenshots

    Presence treatment:

    ![presence](https://github.com/user-attachments/assets/f5b7b9bd-a000-48d8-8aa7-6fd8424d58fb)

    ![self](https://github.com/user-attachments/assets/0eadc479-4dae-4159-bb8d-0b48d31e8673)

    ## Testing

    - `bun test packages/question packages/editor`: 688 passing on the preflight restack. - `bun run types` and `bun run ci`: passing on the preflight restack. - Existing option E2E specs cover the duplicate hint, server fallback, and preserved selection. The browser suite still needs to run on the published head.

  2. MA
    Author

    **Review:** Changes needed

    Checked in a real browser (fake server, two users octocat and hubot, desktop 1440x900 and phone 390x844) plus code and CI.

    **Blocking**

    1. Auto-selecting the added option overwrites another member's shared selection. `question-view.tsx` `onAdded` (~L879) writes `{ mode: "choices", choice: optionId }`. Drafts are shared LWW registers, so I saw hubot select "In SQLite", octocat add "Postgres rows", and the card moved to "Postgres rows" for both users. Hubot's pick was silently replaced by someone else's add. Multiple choice is safe (the new option is merged in; Anchors stayed ticked). The existing contract says the opposite: `e2e/sidecar-option-contracts.e2e.ts:28` ("adding an option ... keeps a selection") and `:76` ("leaves an unanswered decision unselected"), and the AGENTS.md note that an option has a register only once someone selects it. Suggested fix: for single choice, select the new option only when the draft has no choice yet (and likely not at all, to keep the contract); keep the multiple-choice tick, or drop both if the contract stands. If the new behaviour is intended, update those tests and the docs deliberately.

    2. e2e fails (9 tests in run 37692107992), and the PR did not touch `e2e/`. Behaviour-related: `sidecar-option-async.e2e.ts:126` and `:143` (they type "in sqlite" and expect the request to go out; Enter on a duplicate is now blocked client-side, so the held reply never happens), `sidecar-option-contracts.e2e.ts:28`, `:76`, `:180` (expects the `alert` "That option already exists"; now a status hint), `sidecar.e2e.ts:569`, `:840`, `:887` (same duplicate alert and selection assertions). `responsive-comments.e2e.ts:44` may be unrelated or flaky; please confirm on main. Update the duplicate tests to assert the hint "Already an option: ..." and that no request is sent; keep one test for the server-side duplicate alert via a race (another member adds it first).

    **Verified good**

    - Self filtering: `PlanEditor` `user.name` comes from `cursor(handle)` (`packages/editor/src/cursor.ts:36`), so it is the GitHub login, the same value as the presence handles. The comparison is case-insensitive. Octocat's view showed only hubot, hubot's only octocat. - Duplicate rule matches the server (`packages/question/src/schema.ts:280`, trimmed and lowercased). Verified with " in sqlite " and "ON DISK AS MDX". The server alert remains the fallback for races. - No layout shift: Save button y stayed 454.06 with an empty field and with a duplicate. Hint line is reserved. Phone: no horizontal overflow, hint wraps fine. - Max 10: after the tenth option the "Add an option" trigger disappears. Read-only viewers: the trigger is disabled (`onAddOption` is undefined unless `editable`). - Header icon stays at the right edge; presence face sits before the actions.

    **Minor**

    - `questionnaire-controller.ts:381`: `key2` is a poor name, and the reply already carries `option` (`service-append-option.ts` replies with `option`), so `reply.option?.id` avoids re-matching by label. - No unit test for `duplicateOf` or the self filter (pure logic; cheap to cover). - The hint is always reserved (about 1.25rem) while the field is open, so the card is 20px taller than before when the field opens. Acceptable, just note it.

    **CI:** the only expected failure in the format/lint job is the design-contract dynamic-owner hashes (listed in the PR body, safe). e2e is the real blocker above.

  3. MA
    Author

    Addressed: removed select-on-add entirely (onAdded wiring and the option-id return are gone, so the shared draft is never touched). Kept the client duplicate hint and Enter block. Duplicate e2e specs now assert the inline hint and that no question:option frame is sent; a new spec drops option-added frames so only the server can catch a repeat, covering the alert fallback. Added unit tests for duplicateOf and the self filter. e2e passes; responsive-comments only failed once on the first run and passed on later runs (unrelated). Remaining red job is the design-contract hash review noted in the PR body.

  4. MaggieAppletonreferenced this in Show decision evidence as a summary row with objections #361last month on GitHub

  5. MaggieAppletonreferenced this in Show each decision's relationship to the document #367last month on GitHub

  6. MaggieAppletonreferenced this in Mark the Document tab when the Planner writes out of view #372last month on GitHub

  7. MA
    Author

    @copilot the duplicate option label is nice and calmer, but it's not in the right style. Our warning styles need to look consistent, as they did before we made this change. Can you make sure the "already an option" warning at least matches our design system, even if it's only one line long?

  8. Copilotbotmentionedlast month on GitHub

  9. Copilotbotsubscribedlast month on GitHub

  10. MaggieAppletoncopilot-work-startedlast month on GitHub

  11. > @copilot the duplicate option label is nice and calmer, but it's not in the right style. Our warning styles need to look consistent, as they did before we made this change. Can you make sure the "alre...

    Updated the inline hint to use the existing danger surface and text tokens while keeping its reserved line. Committed as `8b7cc7d`.

  12. Copilotbotmentionedlast month on GitHub

  13. Copilotbotsubscribedlast month on GitHub

  14. MaggieAppletoncopilot-work-finishedlast month on GitHub

  15. MaggieAppletonhead-ref-force-pushed 2941b059fde3last month on GitHub

  16. MaggieAppletonrenamed Decision card: inline duplicate hint, select added option, calmer presence to Decision card: inline duplicate warning and calmer presencelast month on GitHub

  17. MaggieAppletonauto-rebase-enabledlast month on GitHub

  18. MaggieAppletonmerged 736cf8d34623last month on GitHub

  19. MaggieAppletonclosedlast month on GitHub

  20. MaggieAppletonhead-ref-deletedlast month on GitHub