fix: guard against nil list model panic in first-run wizard#140
Conversation
When there is only one Basecamp account, the wizard immediately transitions to stepSelectProject and begins loading projects asynchronously. Any message (spinner tick, window resize, key press) arriving before projectsLoadedMsg gets forwarded to the uninitialized (zero-value) list.Model, causing a nil pointer dereference panic in bubbles/list.handleBrowsing. The same race exists for stepSelectAccount with accountList. This fix guards both list Update paths to only forward messages once the list has been populated with items. Easily reproduced on Windows where console resize events arrive with different timing, but the underlying race is platform-independent.
There was a problem hiding this comment.
Pull request overview
This PR fixes a race condition in the first-run wizard that causes nil pointer dereference panics when messages arrive before list models are initialized. The issue occurs when transitioning to list selection steps (account/project) and async data loading completes after UI messages (resize, key press, spinner tick) are received.
Changes:
- Added guards in
FirstRunModel.Update()to prevent forwarding messages to uninitialized list models by checking if lists have items before calling their Update methods - Comprehensive test suite covering panic scenarios, message forwarding with populated lists, step transitions, and view rendering
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| internal/tui/firstrun.go | Added len() > 0 checks to guard accountList and projectList Update calls in stepSelectAccount and stepSelectProject cases |
| internal/tui/firstrun_test.go | New comprehensive test suite covering panic prevention, message handling, step transitions, and view rendering for all wizard steps |
| if len(m.accountList.Items()) > 0 { | ||
| var cmd tea.Cmd | ||
| m.accountList, cmd = m.accountList.Update(msg) | ||
| return m, cmd | ||
| } |
There was a problem hiding this comment.
Consider using a boolean loading flag instead of checking list length for consistency with established patterns. Throughout the codebase (cmd/account/select.go:114-118, cmd/project/select.go:127-131, cmd/todo/select.go), list-based TUI models use an explicit boolean field (e.g., accountsLoading bool) to guard against uninitialized list access. This approach is clearer about intent and more consistent with the codebase conventions.
While len(m.accountList.Items()) > 0 works for the immediate fix, using a loading flag would:
- Make the code's intent more explicit (checking if data is loading vs checking if list has items)
- Be more maintainable and robust to future changes
- Follow the established pattern used in similar select models
| if len(m.projectList.Items()) > 0 { | ||
| var cmd tea.Cmd | ||
| m.projectList, cmd = m.projectList.Update(msg) | ||
| return m, cmd | ||
| } |
There was a problem hiding this comment.
Consider using a boolean loading flag instead of checking list length for consistency with established patterns. Throughout the codebase (cmd/account/select.go:114-118, cmd/project/select.go:127-131, cmd/todo/select.go), list-based TUI models use an explicit boolean field (e.g., projectsLoading bool) to guard against uninitialized list access. This approach is clearer about intent and more consistent with the codebase conventions.
While len(m.projectList.Items()) > 0 works for the immediate fix, using a loading flag would:
- Make the code's intent more explicit (checking if data is loading vs checking if list has items)
- Be more maintainable and robust to future changes
- Follow the established pattern used in similar select models
When there is only one Basecamp account, the first-run wizard immediately transitions to
stepSelectProjectand begins loading projects asynchronously. Any message (spinner tick, window resize, key press) arriving beforeprojectsLoadedMsggets forwarded to the uninitialized (zero-value)list.Model, causing a nil pointer dereference panic inbubbles/list.handleBrowsing.The same race condition exists for
stepSelectAccountwithaccountList.Changes:
accountListandprojectListupdate paths to only forward messages once the list has been populated with itemsThe panic is easily reproduced on Windows where console resize events arrive with different timing, but the underlying race is
platform-independent — any terminal that delivers a message between the step transition and async data load will trigger it.