Skip to content

feat(tasks): filterable branch picker in New Task dialog#131

Merged
johannesjo merged 3 commits into
johannesjo:mainfrom
VitorRibeiroCustodio:feature/filterable-branch-picker
May 21, 2026
Merged

feat(tasks): filterable branch picker in New Task dialog#131
johannesjo merged 3 commits into
johannesjo:mainfrom
VitorRibeiroCustodio:feature/filterable-branch-picker

Conversation

@VitorRibeiroCustodio
Copy link
Copy Markdown
Contributor

@VitorRibeiroCustodio VitorRibeiroCustodio commented May 20, 2026

Summary

Replaces the native <select> base-branch picker in the New Task dialog with a type-to-filter combobox, so users with many branches can narrow the list by typing instead of scrolling.

Closes #129.

Demo

Demo video: to be attached.

Changes

  • src/lib/branch-filter.ts — pure helpers: filterBranches (case-insensitive substring match, prefix matches ordered first) and matchExactBranch.
  • src/components/BranchCombobox.tsx — new combobox component: text input + filtered dropdown, ARIA combobox roles, arrow-key navigation, Enter to select, Esc closes only the dropdown (not the dialog), mouse selection.
  • src/components/NewTaskDialog.tsx — swaps the native <select> for <BranchCombobox>; the section label is now for-linked to the input.
  • openspec/changes/add-filterable-branch-picker/ — OpenSpec change (proposal, tasks, task-creation spec).

Design notes

  • The committed baseBranch value stays the source of truth — the picker only ever commits a branch that exists in the repo.
  • Typed-but-partial text reverts to the previous selection on blur; a fully-typed exact branch name commits.
  • A native keydown listener is used so Esc can stopPropagation and close just the dropdown, leaving the dialog open.
  • No backend or IPC change — branch data still comes from the existing GetBranches channel.

Checks

  • npm run typecheck — pass
  • npm run lint (--max-warnings 0) — pass
  • npm run format:check — pass
  • npm run test — 527 pass (10 new branch-filter tests)
  • openspec validate --all --strict — pass (10/10 items)

Alternatives considered

  • Native <datalist> — would shrink the component to a few lines, but loses control over commit-on-blur, exact-match revert of partial text, and the prefix-before-substring ordering. Rejected for that reason; the custom combobox keeps those semantics explicit.

Notes / open questions

🤖 Generated with Claude Code

The New Task dialog picked the base branch from a native <select>, which
can only be navigated by scrolling. Repos with many branches make finding
the right base branch slow.

Replace it with a type-to-filter combobox: a text input that filters the
branch list by case-insensitive substring (prefix matches first), plus a
dropdown selectable by mouse or keyboard. The committed branch value stays
the source of truth — the picker only ever commits an existing branch.

- Add pure filterBranches/matchExactBranch helpers with unit tests
- Add BranchCombobox component (ARIA combobox, arrow-key nav, Esc closes
  only the dropdown)
- Wire it into NewTaskDialog, replacing the native <select>
- Add OpenSpec change add-filterable-branch-picker

Closes johannesjo#129

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@johannesjo
Copy link
Copy Markdown
Owner

Thanks for taking this on — picker reads cleanly and the pure branch-filter split with tests is the right shape. Below are the items I'd like addressed before promoting from draft; the rest are optional.

Needs changes

1. Enter falls through to form submitsrc/components/BranchCombobox.tsx:122-127

The handler only calls preventDefault() when open() && matches().length > 0. With zero matches (or after Esc has closed the dropdown but the input still has focus), Enter triggers <form onSubmit> and creates the task with whatever baseBranch() was previously committed. The native <select> it replaces never had this behavior. Suggested fix:

} else if (e.key === 'Enter') {
  e.preventDefault();
  if (open() && matches().length > 0) commit(matches()[highlight()]);
  else closeAndResolve();
}

2. Stale text while dropdown is opensrc/components/BranchCombobox.tsx:41-49

The mirror effect only syncs query from props.value while \!open(). If baseBranch updates externally while the dropdown is focused — e.g., the branch list refetch in NewTaskDialog.tsx (project switch / retry path around lines 232–280) — the input keeps stale text and closeAndResolve can revert to a value that's no longer in props.branches.

Cleanest fix is to derive the displayed value reactively instead of writing back into a signal:

const display = createMemo(() => (open() || dirty()) ? query() : props.value);
// <input value={display()} ... />

This also keeps the dirty flag (which is load-bearing — without it, onFocus would show a 1-item filtered list instead of the full list).

3. scrollIntoView runs on mouse hoversrc/components/BranchCombobox.tsx:62-67 + :217

onMouseEnter sets highlight, which fires the scroll effect, which calls scrollIntoView({ block: 'nearest' }). Two side effects:

  • Sweeping the mouse across the open list forces a layout per row.
  • The dropdown is position: absolute inside the dialog's overflow-y: auto panel, so scrollIntoView can scroll the outer dialog itself while the user is hovering options.

Gate the scroll to keyboard-driven highlight changes (e.g. a lastNavSource signal), or set listRef.scrollTop manually from the highlighted item's offset.

Optional

  • src/components/BranchCombobox.tsx:105-137 — the keydown listener has no reactive deps. onMount/onCleanup (or JSX onKeyDown with e.stopPropagation() for Escape isolation — Solid's JSX handlers run before the document-level dialog handler) expresses intent better than createEffect.
  • src/components/BranchCombobox.tsx:213-216onMouseDown preventDefaults on the <li>, but a click on the <ul> padding/scrollbar still blurs the input and commits via closeAndResolve. Adding e.preventDefault() to the <ul> mousedown avoids that.
  • src/components/BranchCombobox.tsx:21loading is a required prop; defaulting it to false would keep the component reusable.
  • src/components/BranchCombobox.tsx:141 — consider maxlength={255} on the input to bound per-keystroke work on adversarial paste (matches typical ref-name limits).
  • src/lib/branch-filter.ts:14-25 — could be a single .filter().sort() (Array.sort is stable in V8) — same complexity, ~8 fewer lines. Cosmetic.
  • src/lib/branch-filter.test.ts — no test asserts ordering when multiple prefix matches exist, nor case-insensitive prefix outranking case-insensitive substring (['Feature/a', 'feature/b'] with query 'fea'). One extra case worth adding.
  • openspec/changes/add-filterable-branch-picker/specs/task-creation/spec.md — declaring the whole task-creation capability via one branch-picker requirement leaves the capability under-specified. Either narrow the name (branch-picker) or accept it'll grow later. Maintainer call.
  • openspec/changes/add-filterable-branch-picker/tasks.md — last task (openspec validate --all --strict) unchecked; the PR body confirms the CLI wasn't available locally. Worth running before archive.

Considered and (mostly) dismissed

  • Native <datalist>: would cut the component to ~5 lines but loses control over commit-on-blur + exact-match revert semantics and the prefix-before-substring sort. Worth one line in the PR description explaining the rejection — not a blocker.
  • <Index> vs <For>: <For> keyed by string already reuses retained items when the filter narrows; <Index> would force more re-renders, not fewer. Keep <For>.
  • Generalize to Combobox<T> now: YAGNI — single call site. Defer until ProjectSelect.tsx (or similar) migrates.
  • Inline-style duplication with other inputs: matches existing convention in NewTaskDialog.tsx / BranchPrefixField.tsx. Out of scope for this PR.

Verified non-issues

XSS, prototype pollution, IPC trust boundary — none. SolidJS escapes branch names rendered as text-node children; the picker only ever commits a value from the server-supplied list (matchExactBranch requires exact match); backend validateBranchName already rejects shell metas / control chars at every IPC entry that consumes baseBranch.

Happy to re-review once W1–W3 are addressed.

@VitorRibeiroCustodio VitorRibeiroCustodio marked this pull request as ready for review May 20, 2026 14:40
@VitorRibeiroCustodio
Copy link
Copy Markdown
Contributor Author

VitorRibeiroCustodio commented May 20, 2026

Thanks for the thorough review! All three blocking items plus the optionals are addressed in e9066f3.

Needs changes

  1. Enter falls throughEnter now always preventDefault()s; commits the highlight when the list is open and non-empty, otherwise calls closeAndResolve(). The branch field can no longer submit the form.
  2. Stale text — dropped the write-back mirror effect; the displayed text is now derived (display = createMemo(() => (open() || dirty()) ? query() : props.value)), so an external baseBranch change can no longer leave stale text. query is seeded on focus to keep the dirty flag behavior intact.
  3. scrollIntoView on hover — replaced with manual listRef.scrollTop math (offsetTop/offsetHeight), confined to the listbox so the dialog never scrolls, and gated behind a keyboardNav flag that onMouseEnter clears — hover no longer triggers any scroll.

Optional — applied

  • keydown listener moved to onMount/onCleanup
  • e.preventDefault() added to the <ul> mousedown (padding/scrollbar clicks no longer blur+commit)
  • loading is now optional, defaults to false
  • maxlength={255} on the input
  • filterBranches simplified to a single .filter().sort()
  • added branch-filter tests for multiple-prefix ordering and case-insensitive prefix outranking substring
  • OpenSpec capability renamed task-creationbranch-picker
  • <datalist> rejection noted in the PR description

Ready for re-review whenever you have a moment.

@VitorRibeiroCustodio VitorRibeiroCustodio force-pushed the feature/filterable-branch-picker branch from e9066f3 to 63ba609 Compare May 20, 2026 15:33
@VitorRibeiroCustodio
Copy link
Copy Markdown
Contributor Author

Update: openspec validate --all --strict now passes — installed @fission-ai/openspec and ran it locally, 10/10 items valid (including change/add-filterable-branch-picker). The last task in tasks.md is now checked, and the PR description Checks section is updated accordingly.

All blocking and optional items from the review are addressed and pushed (63ba609). Ready for re-review.

@johannesjo
Copy link
Copy Markdown
Owner

Thanks for the updates. I re-reviewed the current head and found a few interaction issues that still look worth fixing before merge.

Needs changes

  1. src/components/BranchCombobox.tsx:104

onFocus() seeds the input with the committed branch but does not select or clear that text. In the common path where the field shows main, clicking/focusing it and typing feature produces mainfeature, so filtering does not work unless the user manually selects/deletes the existing value first. For a type-to-filter picker, focus should likely select the current text or start with an empty search query.

  1. src/components/BranchCombobox.tsx:235

Mouse selection uses mousedown.preventDefault(), so the input keeps focus after commit() closes the dropdown. Because reopening is only wired through focus/input/keyboard events, clicking the already-focused input again does not reopen the list; mouse users have to blur/refocus or use ArrowDown. Adding an input click/mousedown reopen path should fix this.

  1. src/components/NewTaskDialog.tsx:255, src/components/BranchCombobox.tsx:54, src/components/NewTaskDialog.tsx:518

During branch refetch/project switch, the dialog clears branches() and sets loading, but leaves baseBranch() unchanged until the async fetch completes. The new combobox renders that stale props.value while closed, and submit is not blocked by branchesLoading(), so the user can create a task with the previous project’s branch in that loading window. This stale payload path may have existed before, but the new disabled text input now visibly presents the stale branch as the current value. Please clear the committed branch during reload or block submit until branch loading completes.

Low-risk edge case

  • src/components/BranchCombobox.tsx:133: ArrowDown with zero matches can set highlight to -1; if matches later repopulate while the popup remains open, Enter can commit matches()[-1]. Clamp the lower bound or guard the Enter commit.

Verified locally: npm test, npm run typecheck, npm run lint, npm run format:check, and openspec validate --all --strict all pass.

Address re-review feedback on PR johannesjo#131:

- Focus now opens the list with an empty search query instead of
  seeding the committed branch into the input, so typing filters
  instead of appending (no more "mainfeature").
- Add an input click handler to reopen the dropdown after a mouse
  commit keeps focus on the input.
- Clear the committed base branch during branch reload and block
  submit while branches load or after a failed fetch (inline error +
  Retry), so a task can't be created with a stale/empty base branch.
- Clamp the highlighted index lower bound to 0 so ArrowDown with zero
  matches can't later commit matches()[-1].

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@VitorRibeiroCustodio
Copy link
Copy Markdown
Contributor Author

Thanks for the re-review. All four items addressed in 5fb8baf.

Needs changes

  1. mainfeature on focus — focus now opens the list with an empty search query rather than seeding the committed branch into the input, so the first keystroke starts a fresh filter. The committed branch is still shown when the field is closed (via the derived display memo) and is highlighted in the open list. dirty stays false until the user types, so the list still opens showing every branch.

  2. No reopen after mouse commit — added an input onClick handler that reopens the dropdown. Mouse commits keep focus on the input (option uses mousedown + preventDefault), so a later focus event never fires; the click path covers that case.

  3. Stale branch during refetch — the branch-fetch effect now clears baseBranch() at the start of a reload, and submit is blocked while branchesLoading() is true. On a failed fetch the combobox is swapped for an inline error + Retry, and branchOk additionally requires !branchesError(), so a task can't be created with a stale or empty base branch in the loading/error window.

Low-risk edge case

  • ArrowDown with zero matches can no longer leave highlight at -1 — index math goes through a clampHighlight(index, count) helper that pins an empty list to 0, so matches()[highlight()] is always safe even if matches repopulate while the popup is open. Added unit tests for it.

Verified locally: npm test (536 passing), npm run typecheck, npm run lint, npm run format:check, and openspec validate --all --strict (10/10) all pass.

Ready for re-review whenever you have a moment.

@johannesjo
Copy link
Copy Markdown
Owner

Thank you very much! <3

@johannesjo
Copy link
Copy Markdown
Owner

johannesjo commented May 21, 2026

Thanks — all four items are addressed cleanly. Verified at 5fb8baf:

  • Focus now seeds an empty query; typing no longer concatenates onto the committed branch.
  • Input onClick reopens the dropdown after a mouse commit keeps focus on the input.
  • Branch refetch clears baseBranch up front and submit is gated on \!branchesLoading() && (isNonGitProject() || (\!\!baseBranch() && \!branchesError())). The inline error + Retry replaces the stale-toast path.
  • clampHighlight pins the index at 0 for an empty list, so ArrowDown → repopulate → Enter can no longer hit matches()[-1]. Unit tests cover the regression case directly.

Pure helpers (clampHighlight, resolveOnBlur) extracted with full coverage, the display memo replaces the mirror-write, and the comments at the non-obvious spots (mouse-vs-keyboard keyboardNav, empty-seed rationale, label for nulling) all explain why — appreciated.

One tiny follow-up, not a blocker: after a successful Retry, focus is on the now-unmounted Retry button, so the user has to click the field to interact. A inputRef.focus() on combobox mount (or once branchesLoading flips back to false post-retry) would smooth that. Happy to leave it for a later PR.

@johannesjo johannesjo merged commit ce8f97f into johannesjo:main May 21, 2026
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Filterable branch picker in New Task dialog

2 participants