Issue close controls: split buttons — one-click Close (completed) with not-planned dropdown segment; Comment and Close likewise; outside-click dismisses the menu #311

New issue

Closed

opened 2026-09-10 22:10:10 +00:00 by crueber · 3 comments

crueber commented 2026-09-10 22:10:10 +00:00

Owner

Copy link

What's requested

Rework the close affordances on an issue into split buttons (primary action + dropdown segment), matching GitHub's pattern:

1. Close — clicking the button closes immediately (one click, completed by default); a dropdown segment offers "Close as not planned" (the incomplete variant).
2. Comment and Close — clicking posts the comment and closes as completed by default; a dropdown segment offers "Comment and close as not planned".
3. Clicking anywhere outside an open dropdown dismisses it (currently only Escape/Tab dismiss — clicking elsewhere leaves it open).

Current state (code evidence)

Everything lives in web/src/components/CommentComposer.jsx:

• ChooserMenu (:23-110) is the dropdown primitive. When closeChooser is set, both Close and Comment and Close render as pure chooser buttons (:236-262) — you must open the menu and pick a reason to close at all. There is no one-click path.
• Reasons are exactly two: CLOSE_COMPLETED = "completed" and CLOSE_NOT_PLANNED = "not_planned" (web/src/lib/issue-events.js:29-30), validated client-side (:103) and carried as state_reason server-side (internal/issues/model.go:70). The user's "incomplete" maps to the existing not_planned — no backend or reason-vocabulary change is needed; verify the event-text rendering (issue-events.js:82) reads sensibly for both paths.
• The menu has keyboard dismissal only: Escape (ChooserMenu onMenuKey) and Tab-out; no document-level outside-click listener. The repo already has the standard pattern to copy — RefPicker/TasksOverlay in web/src/pages/Repo.jsx (document click + !root.contains(e.target) + onCleanup removal), and #255 fixed the clone menu with the same shape.
• The menu opens upward (bottom-full, :92) beneath the composer — keep that.

Proposed design

1. Split the ChooserMenu into a split-button: [primary label][▾] as one visually joined control. Primary click = default action; ▾ segment opens the menu.
   • Close: primary = Close → runClose(CLOSE_COMPLETED); menu item = Close as not planned → runClose(CLOSE_NOT_PLANNED). Menu no longer needs a "Close as completed" item (the button IS that), but keeping it harms nothing — implementer's call, note it in the PR.
   • Comment and Close: primary = posts the current body + runClose(CLOSE_COMPLETED) (via the existing commentAndClose(body, reason) — body may be empty; preserve current semantics where an empty body still closes); menu item = "Comment and close as not planned".
   • Keep the closeChooser=false fallback (plain button) for callers that don't opt into reasons (PR close has no reasons today) — only the chooser path changes.
2. Outside-click dismissal on the open menu: document click listener closing when the target is outside both the menu and the toggle (the split button's two segments!), removed in onCleanup. Must not close-then-reopen when clicking the ▾ segment itself (segment is inside the boundary).
3. Accessibility preserved: aria-haspopup, aria-expanded, menu role, arrow-key navigation, Escape-with-focus-return (all exist in ChooserMenu — carry them into the split variant). The primary segment needs aria-label distinguishing it from the menu toggle.
4. Busy state: both segments disable together during an in-flight close (existing getBusy() — keep single-flight).

Acceptance criteria

• One click on "Close" closes the issue as completed — no menu.
• "Close ▾" offers "Close as not planned" which closes with state_reason: not_planned.
• "Comment and Close" posts the composer body and closes as completed in one click (empty body closes without posting a comment, matching current behavior).
• "Comment and Close ▾" offers "Comment and close as not planned".
• An open dropdown closes on any click outside it (including on the composer body and page background), and does NOT toggle-close when clicking its own ▾ segment.
• Reopen path unchanged (plain button, no reason).
• Keyboard support intact: menu arrow navigation, Escape closes with focus returned to the toggle, primary segments operable via Enter/Space.
• Both segments disable while a close is in flight; no double-close.
• Event timeline renders "closed as completed"/"closed as not planned" correctly for all four paths (close ×2, comment-and-close ×2).
• Headless test for the split-button state machine (open/close/dismiss/outside-click) per the headless-module convention; visual check light/dark.

## What's requested Rework the close affordances on an issue into **split buttons** (primary action + dropdown segment), matching GitHub's pattern: 1. **Close** — clicking the button closes **immediately** (one click, completed by default); a dropdown segment offers "Close as not planned" (the incomplete variant). 2. **Comment and Close** — clicking posts the comment and closes **as completed** by default; a dropdown segment offers "Comment and close as not planned". 3. Clicking **anywhere outside** an open dropdown dismisses it (currently only Escape/Tab dismiss — clicking elsewhere leaves it open). ## Current state (code evidence) Everything lives in `web/src/components/CommentComposer.jsx`: - `ChooserMenu` (:23-110) is the dropdown primitive. When `closeChooser` is set, **both** Close and Comment and Close render as pure chooser buttons (:236-262) — you must open the menu and pick a reason to close at all. There is no one-click path. - Reasons are exactly two: `CLOSE_COMPLETED = "completed"` and `CLOSE_NOT_PLANNED = "not_planned"` (`web/src/lib/issue-events.js:29-30`), validated client-side (:103) and carried as `state_reason` server-side (`internal/issues/model.go:70`). The user's "incomplete" maps to the existing `not_planned` — **no backend or reason-vocabulary change is needed**; verify the event-text rendering (`issue-events.js:82`) reads sensibly for both paths. - The menu has keyboard dismissal only: Escape (ChooserMenu `onMenuKey`) and Tab-out; **no document-level outside-click listener**. The repo already has the standard pattern to copy — `RefPicker`/`TasksOverlay` in `web/src/pages/Repo.jsx` (document click + `!root.contains(e.target)` + `onCleanup` removal), and #255 fixed the clone menu with the same shape. - The menu opens *upward* (`bottom-full`, :92) beneath the composer — keep that. ## Proposed design 1. **Split the ChooserMenu into a split-button**: `[primary label][▾]` as one visually joined control. Primary click = default action; ▾ segment opens the menu. - Close: primary = `Close` → `runClose(CLOSE_COMPLETED)`; menu item = `Close as not planned` → `runClose(CLOSE_NOT_PLANNED)`. Menu no longer needs a "Close as completed" item (the button IS that), but keeping it harms nothing — implementer's call, note it in the PR. - Comment and Close: primary = posts the current body + `runClose(CLOSE_COMPLETED)` (via the existing `commentAndClose(body, reason)` — body may be empty; preserve current semantics where an empty body still closes); menu item = "Comment and close as not planned". - Keep the `closeChooser=false` fallback (plain button) for callers that don't opt into reasons (PR close has no reasons today) — only the chooser path changes. 2. **Outside-click dismissal** on the open menu: document click listener closing when the target is outside both the menu and the toggle (the split button's two segments!), removed in `onCleanup`. Must not close-then-reopen when clicking the ▾ segment itself (segment is inside the boundary). 3. **Accessibility preserved**: `aria-haspopup`, `aria-expanded`, menu role, arrow-key navigation, Escape-with-focus-return (all exist in `ChooserMenu` — carry them into the split variant). The primary segment needs `aria-label` distinguishing it from the menu toggle. 4. **Busy state**: both segments disable together during an in-flight close (existing `getBusy()` — keep single-flight). ## Acceptance criteria - [ ] One click on "Close" closes the issue as completed — no menu. - [ ] "Close ▾" offers "Close as not planned" which closes with `state_reason: not_planned`. - [ ] "Comment and Close" posts the composer body and closes as completed in one click (empty body closes without posting a comment, matching current behavior). - [ ] "Comment and Close ▾" offers "Comment and close as not planned". - [ ] An open dropdown closes on any click outside it (including on the composer body and page background), and does NOT toggle-close when clicking its own ▾ segment. - [ ] Reopen path unchanged (plain button, no reason). - [ ] Keyboard support intact: menu arrow navigation, Escape closes with focus returned to the toggle, primary segments operable via Enter/Space. - [ ] Both segments disable while a close is in flight; no double-close. - [ ] Event timeline renders "closed as completed"/"closed as not planned" correctly for all four paths (close ×2, comment-and-close ×2). - [ ] Headless test for the split-button state machine (open/close/dismiss/outside-click) per the headless-module convention; visual check light/dark.

crueber added the

issues

label 2026-09-10 22:20:49 +00:00

crueber added this to the v1 milestone 2026-09-10 22:20:49 +00:00

crueber referenced this issue from a commit 2026-09-10 22:58:50 +00:00

Fix #311: split-button close controls (one-click Close-as-completed + not-planned menu + outside-click dismiss)

crueber referenced this issue from a pull request that will close it, 2026-09-10 22:58:52 +00:00

Fix #311: split-button close controls #315

crueber commented 2026-09-10 22:58:54 +00:00

Author

Owner

Copy link

PR #315 (fix/issue-311) implements the split-button close controls — ready for review, not merged.

PR #315 (fix/issue-311) implements the split-button close controls — ready for review, not merged.

crueber referenced this issue from a commit 2026-09-10 23:20:36 +00:00

Fix #311 review: drop inaccurate incidental-fix claim in 08_ui_sdk.md

crueber commented 2026-09-10 23:20:41 +00:00

Author

Owner

Copy link

PR #315 review (split-button close controls) — verified in scratch worktree, fix pushed as a9a3911.

ACCEPTANCE (all pass, CommentComposer.jsx):

• One-click Close as completed: onPrimary={() => runClose(CLOSE_COMPLETED)} (:263) — explicit reason sent, not the API default. Comment-and-Close likewise posts body + CLOSE_COMPLETED (:292).
• Dropdown holds ONLY the not-planned alternate (:260-261, :289-290) — within the issue's latitude ('implementer's call'), noted in 08_ui_sdk.md.
• Comment-and-Close empty-body semantics preserved: Issue.jsx:146 posts only when non-empty, then closes — empty body just closes.
• Outside-click dismiss: document click listener + !root.contains(e.target) (:61-65), removed in onCleanup. No toggle fight: ▾ clicks are inside the root boundary so the doc handler ignores them; open-then-click-▾ toggles shut while the doc handler no-ops (getOpen() false).
• Esc refocuses toggle (close(true), :72/:93), Tab-out dismisses without choosing (:79-82); menu roles + arrow nav intact (:111-112, :122, :131, :73-78).
• Busy-disable on all three controls: disabled={props.disabled} x3 (:99, :114, :133), fed by getBusy() (:262, :291); runClose/commentAndClose re-guard on getBusy() (:206, :219).
• closeChooser=false fallback intact (:248-254, :269-283); reopen stays plain button via onClose without chooser.
• Event text: issueEventText state_changed closed/completed -> 'closed as completed', not_planned -> 'closed as not planned' (issue-events.js:82-84); closePatch throws on missing reason (:102-107).
• No backend change (diff: 08_ui_sdk.md + CommentComposer.jsx + split-close.test.js only); state_reason vocabulary untouched; no new deps (only added import is onCleanup from solid-js); dark+light menu-item classes unchanged (hover:bg-zinc-100 dark:hover:bg-zinc-800, :132).

ONE FINDING (fixed, pushed a9a3911): 08_ui_sdk.md claimed an 'incidental fix' where the Comment-and-Close chooser passed props.disabled (always undefined) instead of getBusy() — inaccurate: main already passed disabled={getBusy()} to BOTH chooser menus. Doc now states the kept behavior (all three split controls disable from getBusy(), as the choosers did). Code was already correct; no behavior change.

VERIFY: full node suite in scratch 595/595 pass (588 existing + 7 new split-close); vite build clean in 2s (chunk-size warning is pre-existing). No browser (per instructions; change is headless-testable + reasoned). Main worktree untouched.

MERGE RECOMMENDATION: ready to merge.

PR #315 review (split-button close controls) — verified in scratch worktree, fix pushed as a9a3911. ACCEPTANCE (all pass, CommentComposer.jsx): - One-click Close as completed: onPrimary={() => runClose(CLOSE_COMPLETED)} (:263) — explicit reason sent, not the API default. Comment-and-Close likewise posts body + CLOSE_COMPLETED (:292). - Dropdown holds ONLY the not-planned alternate (:260-261, :289-290) — within the issue's latitude ('implementer's call'), noted in 08_ui_sdk.md. - Comment-and-Close empty-body semantics preserved: Issue.jsx:146 posts only when non-empty, then closes — empty body just closes. - Outside-click dismiss: document click listener + !root.contains(e.target) (:61-65), removed in onCleanup. No toggle fight: ▾ clicks are inside the root boundary so the doc handler ignores them; open-then-click-▾ toggles shut while the doc handler no-ops (getOpen() false). - Esc refocuses toggle (close(true), :72/:93), Tab-out dismisses without choosing (:79-82); menu roles + arrow nav intact (:111-112, :122, :131, :73-78). - Busy-disable on all three controls: disabled={props.disabled} x3 (:99, :114, :133), fed by getBusy() (:262, :291); runClose/commentAndClose re-guard on getBusy() (:206, :219). - closeChooser=false fallback intact (:248-254, :269-283); reopen stays plain button via onClose without chooser. - Event text: issueEventText state_changed closed/completed -> 'closed as completed', not_planned -> 'closed as not planned' (issue-events.js:82-84); closePatch throws on missing reason (:102-107). - No backend change (diff: 08_ui_sdk.md + CommentComposer.jsx + split-close.test.js only); state_reason vocabulary untouched; no new deps (only added import is onCleanup from solid-js); dark+light menu-item classes unchanged (hover:bg-zinc-100 dark:hover:bg-zinc-800, :132). ONE FINDING (fixed, pushed a9a3911): 08_ui_sdk.md claimed an 'incidental fix' where the Comment-and-Close chooser passed props.disabled (always undefined) instead of getBusy() — inaccurate: main already passed disabled={getBusy()} to BOTH chooser menus. Doc now states the kept behavior (all three split controls disable from getBusy(), as the choosers did). Code was already correct; no behavior change. VERIFY: full node suite in scratch 595/595 pass (588 existing + 7 new split-close); vite build clean in 2s (chunk-size warning is pre-existing). No browser (per instructions; change is headless-testable + reasoned). Main worktree untouched. MERGE RECOMMENDATION: ready to merge.

crueber closed this issue 2026-09-10 23:21:01 +00:00

crueber commented 2026-09-10 23:21:02 +00:00

Author

Owner

Copy link

Fixed by PR #315 incl. review doc fix (one-click completed + not-planned segments + outside-click; 595/595), merged. Closing.

Fixed by PR #315 incl. review doc fix (one-click completed + not-planned segments + outside-click; 595/595), merged. Closing.

crueber referenced this issue from a commit 2026-09-10 23:21:03 +00:00

Merge pull request 'Fix #311: split-button close controls' (#315) from fix/issue-311 into main

crueber referenced this issue 2026-09-11 12:05:10 +00:00

Milestone reassignment shows stale membership in milestone-filtered issue lists until reload (client cache invalidation gap) #318

Sign in to join this conversation.

No Branch/Tag specified

Branches Tags

main

fix/pac-56-oidc-hardening

fix/issue-631

fix/issue-629

fix/issue-627

fix/issue-625

fix/issue-623

fix/issue-621

fix/issue-619

fix/issue-617

fix/issue-613

fix/issue-612

fix/issue-611

fix/issue-605

fix/issue-602

fix/issue-601

fix/issue-599

fix/issue-598

fix/issue-597

fix/issue-594

fix/issue-595

fix/issue-592

fix/issue-588

fix/issue-587

fix/issue-586

fix/issue-584

fix/issue-581

fix/issue-580

fix/issue-575

fix/issue-574

fix/issue-573

fix/issue-572

fix/issue-568

fix/issue-567

fix/issue-566

fix/issue-564

fix/issue-561

fix/issue-560

fix/issue-557

fix/issue-555

fix/issue-554

fix/issue-550

fix/issue-547

fix/issue-546

fix/issue-545

fix/issue-544

fix/issue-537

fix/issue-527

fix/issue-531

fix/issue-529

fix/issue-539

fix/issue-525

fix/issue-533

fix/issue-530

fix/issue-522

fix/issue-521

fix/issue-520

fix/issue-518

fix/issue-517

fix/issue-512-rail

fix/issue-513-etag

fix/issue-513

fix/issue-512

fix/issue-506

fix/issue-505

fix/issue-504

fix/issue-503

fix/issue-502

fix/issue-498

fix/issue-497

fix/issue-495

fix/issue-487

fix/issue-491

fix/issue-485

fix/issue-484

fix/issue-486

fix/issue-482

fix/issue-481

fix/issue-479

fix/issue-477

fix/issue-466

fix/issue-465

fix/issue-464

fix/issue-463

fix/issue-455

fix/issue-460

fix/issue-459

fix/issue-458

fix/issue-457

fix/issue-456

fix/issue-451

fix/issue-450

fix/issue-447

fix/issue-445

fix/issue-444

fix/issue-442

fix/issue-440

fix/issue-438

fix/issue-437

fix/issue-435

fix/issue-430

fix/issue-432

fix/issue-424

fix/issue-423

fix/issue-422

fix/issue-421

fix/issue-420

fix/issue-419

fix/issue-416

fix/issue-415

fix/issue-413

fix/issue-410

fix/issue-409

fix/issue-405

fix/issue-403

fix/issue-401

fix/issue-397

fix/issue-396

fix/issue-395

fix/issue-394

fix/issue-390

fix/issue-391

fix/issue-382

fix/issue-385

fix/issue-384

fix/issue-380

fix/issue-381

fix/issue-376

fix/issue-374

fix/issue-364

fix/issue-371

fix/issue-370

fix/issue-363

fix/issue-362

fix/issue-361

fix/issue-360

fix/issue-359

fix/issue-358

fix/issue-347

fix/issue-346

fix/issue-348

fix/issue-340

fix/issue-345

fix/issue-344

fix/issue-339

fix/issue-338-sim

fix/issue-337

fix/issue-334

fix/issue-332

fix/issue-328

fix/issue-325

fix/issue-324

fix/issue-323

fix/issue-320

fix/issue-319

fix/issue-318

fix/issue-314

fix/issue-312

fix/issue-311

fix/issue-310

fix/issue-307

fix/issue-301

fix/issue-295

fix/issue-300

docs/issue-288

fix/issue-286

fix/issue-285

fix/issue-283

fix/issue-281

fix/issue-274

fix/issue-273

fix/issue-272

fix/issue-271

fix/issue-280

fix/issue-278

fix/issue-289

fix/issue-277

fix/issue-276

fix/issue-275

fix/issue-270

fix/issue-234

fix/issue-251

fix/issue-263

fix/issue-259

fix/issue-255

fix/issue-254

fix/issue-253

fix/issue-252

feat/issue-247

feat/issue-248

fix/issue-244

fix/issue-243

No results found.

Labels

Clear labels   

actions

  

bug

Something is not working

  

cli

  

duplicate

This issue or pull request already exists

  

enhancement

New feature

  

fork

  

forum

  

git storage

  

help wanted

Need some help

  

insights

  

invalid

Something is wrong

  

issues

  

moderation

  

oidc

  

ownership transfer

  

packages

  

pr/merge protection rules

  

projects

  

pull requests

  

question

More information is needed

  

releases

  

sponsorships

  

tags

  

webhooks

  

wiki

  

wontfix

This won't be fixed

No labels

actions

bug

cli

duplicate

enhancement

fork

forum

git storage

help wanted

insights

invalid

issues

moderation

oidc

ownership transfer

packages

pr/merge protection rules

projects

pull requests

question

releases

sponsorships

tags

webhooks

wiki

wontfix

Milestone

Clear milestone

No items

No milestone

v1

Projects

Clear projects

No items

No project

Assignees

Clear assignees

No assignees

1 participant

Notifications

Subscribe

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set

Reference

crueber/walhub#311

No description provided.