chore: replace AlecAivazis/survey with charmbracelet/bubbletea

[DanielleMaywood]

fixes #12720

This PR replaces the unmaintained https://github.com/AlecAivazis/survey library with https://github.com/charmbracelet/bubbletea.

The existing tests for the components Select, RichSelect and MultiSelect do not test anything meaningful as the components short-circuit to return the first option when ran in a test and the tests just confirm this short-circuiting behaviour. Future work will be to remove this short-circuiting behaviour and fix the tests.

I've manually tested the behaviour of the components with:

• go run ./cmd/cliui select
  • Select
  • Before: https://asciinema.org/a/hPVc4qk95PK1qzCbMOWSDlP5a
  • After: https://asciinema.org/a/0Gzzhza6e3sleIhLnLvTBJSY5
• go run ./cmd/coder exp prompt-example select
  • Select
  • Before: https://asciinema.org/a/kKw91eDUrMF2uGh1hdusK3FQm
  • After: https://asciinema.org/a/RkNwryAJ2zf4W7z4m0kABu45I
• go run ./cmd/coder exp prompt-example multiple
  • Select, MultiSelect
  • Before: https://asciinema.org/a/kcQu4dfKLAMEcaMbD4nVdK5MN
  • After: https://asciinema.org/a/gR4SRwqWbuoyvLt1BpCOeYAKr
• go run ./cmd/coder exp prompt-example multi-select
  • MultiSelect
  • Before: https://asciinema.org/a/uIUyOw2ITEThRzk0JVwJfl7Kv
  • After: https://asciinema.org/a/LWo5r7VQ5X95wHlhWaowbyqXn
• go run ./cmd/coder exp prompt-example rich-parameter
  • RichSelect
  • Before: https://asciinema.org/a/yLPOdVKglxiEDdCzMn847O3Zl
  • After: https://asciinema.org/a/QCdmECsboRPWykhp2uKu4Yjgz
• cdr templates delete (against a local instance)
  • Select
  • Before: https://asciinema.org/a/2C3E4H5YtgDkgdRVY49aVGYHz
  • After: https://asciinema.org/a/W3kzC8R5r7T9o3NDH26lxl4FL
• cdr orgs roles edit organization-auditor (against a local instance)
  • Select, MultiSelect
  • Before: https://asciinema.org/a/cP3TTNmAjYGSy4RLt97W9bATo
  • After: https://asciinema.org/a/eKQlkL9C57Fl9VT7R9Y1Yx6Gu
• cdr create -t devcontainer-docker (against a local instance)
  • RichSelect
  • Before: https://asciinema.org/a/jLBTCWTIM5R5RTxX4OPwlsI3J
  • After: https://asciinema.org/a/aL6pJhnKEoIOorlCzd0Y4wyNs

[johnstcn]

nit: invert and early return

[DanielleMaywood]

I've slightly changed the logic here. I just noticed it is slightly different to how the func (m selectModel) Update function works. m.search.Update can handle more than just a tea.KeyMsg. With the updated logic does this still make sense?

[mafredri]

Nice work, and thanks for the asciinemas! Left a few comments on changes I'd like to see in all functions/models, but otherwise looks good.

[mafredri]

I found the blinking fairly distracting in the aciinemas, that behavior seemed different from the original. Is this what controls it and should we actually blink?

[DanielleMaywood]

Yes, this is what controls it https://pkg.go.dev/github.com/charmbracelet/bubbles@v0.19.0/textinput#Blink.

I'm happy to back it out

[DanielleMaywood]

Doing some more research, it appears that the textinput component also triggers the blinking behaviour

[mafredri]

What happens if the program receives a kill -INT $PID while tea is active? Do we handle it nicely?

[DanielleMaywood]

Unless I'm doing something wrong, it appears nothing happens?

[mafredri]

Hmm, did you try with a coder command or cliui test binary? With coder I would expect the program to exit with a cancelled.

[DanielleMaywood]

I used the cliui binary with go run ./cmd/cliui select. I shall try against the coder binary

[johnstcn]

I checked on main and this behaviour appears to be the status quo.

[DanielleMaywood]

Just tested and it does not! Need to handle that then, err is nil

[DanielleMaywood]

So... Bubbletea listens for SIGINT and SIGTERM signals and emits a QuitMsg command
https://github.com/charmbracelet/bubbletea/blob/e58efab3713e8f924e90a416e66326b23253f7ab/tea.go#L238-L272

The issue is, the event loop finishes on a QuitMsg command without notifying our Update function.
https://github.com/charmbracelet/bubbletea/blob/e58efab3713e8f924e90a416e66326b23253f7ab/tea.go#L352-L355

We could potentially install our own signal handlers by disabling the builtin one
https://pkg.go.dev/github.com/charmbracelet/bubbletea#WithoutSignalHandler
https://github.com/charmbracelet/bubbletea/blob/e58efab3713e8f924e90a416e66326b23253f7ab/tea.go#L508-L511

[DanielleMaywood]

I can confirm by disabling the builtin signal handler it is possible to get err to contain the Canceled error.

I wrote a small signal handler

type terminateMsg struct{}

func installSignalHandler(p *tea.Program) chan struct{} {
	ch := make(chan struct{})

	go func() {
		sig := make(chan os.Signal, 1)
		signal.Notify(sig, syscall.SIGINT, syscall.SIGTERM)

		defer func() {
			signal.Stop(sig)
			close(ch)
		}()

		for {
			select {
			case <-ch:
				return

			case <-sig:
				p.Send(terminateMsg{})
			}
		}
	}()

	return ch
}

Then install it on a *tea.Program

	ch := installSignalHandler(p)
	defer func() {
		ch <- struct{}{}
	}()

And handle it in the Update methods

	switch msg := msg.(type) {
	case terminateMsg:
		m.canceled = true
		return m, tea.Quit

If this seems okay I can push this change? @mafredri

[mafredri]

Might I suggest returning a func instead of the channel for close/deregistering the handler? I feel the usage will be a bit more idiomatic/easy to understand.

On the other hand, perhaps we don't need to handle signals at all and leave it to be the responsibility of the coder command that needs it? I guess it's possible the terminal is left in a bad state unless the signal is handled though (not sure if it enters raw mode or not).

PS. You might want to only handle signal.Interrupt for the Windows compatibility.

[DanielleMaywood]

Yes, definitely a good idea. Have made that change 👍

Killing the process with kill -TERM $PID leaves the cursor in a bad state if we don't explicitly handle it (syscall.SIGTERM).

Will push these changes up, can always revert if unhappy.

[johnstcn]

Nice work and thorough testing!

[mafredri]

LGTM after the conflicts are resolved, nice work!

[mtojek]

LGTM 👍 Feel free to merge once you update flake.nix go.mod and make CI happy. Nice contribution!

[Emyrk]

Late to the party, but one thing that bothered me with the survey package was that the prompt question remained after it was answered. I wonder if we can fix that now.

[DanielleMaywood]

Late to the party, but one thing that bothered me with the survey package was that the prompt question remained after it was answered. I wonder if we can fix that now.

This absolutely can be changed now! Personally I prefer the question (with the answer) remaining, so I'd be curious if there is any way of finding out if that would be a positive/negative change amongst our customers.