search

charmbracelet / huh

Build terminal forms and prompts 🤷🏻‍♀️
MIT License
3.72k stars 94 forks source link

fix: Fix aborting a form returning a timeout error #287

Closed Sculas closed 6 days ago

Sculas commented 2 weeks ago

This PR fixes an issue caused by #276 where aborting a form would return ErrTimeout instead, because when f.timeout == 0, time.Since(startTime) >= f.timeout is always true. This was discussed here: https://github.com/charmbracelet/huh/pull/276#discussion_r1646225089

This is fixed by simply checking if the error returned by bubbletea is ErrProgramKilled, which is already handled by tea.WithContext(ctx), making the (erroneous) check unnecessary anyway.

I've also added tests for handling timeouts, user aborts, and making sure ErrUserAborted is returned instead of ErrTimeout when a timeout error occurs (the issue mentioned above). This required an extra field program for testing to hold the tea.Program to send messages to the Form (batchUpdate cannot be used here because it's managed by tea.Program). RunWithContext(context.Context) has been added to support this.

Sculas commented 2 weeks ago

@maaslalani Are you sure you don't have a timeout set? As the PR mentions, this only occurs when f.timeout == 0 (so no timeout set).

Looks like tests are failing. I'll look into it when I'm back.

maaslalani commented 2 weeks ago

Sorry you're totally right, I was on an incorrect branch!

Sculas commented 2 weeks ago

I've applied the changes you requested and removed the program field by introducing RunWithContext(context.Context), which made sense anyway because the timeout used context.Background() instead of accepting an existing context. Of course, Run() is kept and just calls RunWithContext(context.Background()) as it did previously as well.

I've fixed the data race as well and all tests now pass with -race.

Sculas commented 6 days ago

@maaslalani Would you be able to look into this soon?

maaslalani commented 6 days ago

@Sculas This looks great! Thank you!