Bugfix 142/toast notifications go away #146

Merged

noe.trevino merged 2 commits from bugfix-142/toast-notifications-go-away into main

2026-04-15 22:36:32 -05:00

noe.trevino commented

2026-04-15 22:27:10 -05:00

Owner

Checklist

Read CONTRIBUTING.md
Ran tests (go test ./... -race) locally
Formatted and linted with gofmt -w . and golangci-lint run ./... respectively
Added tests if necessary (significant changes/complex logic)
Updated documentation if applicable
Updated README.md if applicable

Summary

## Checklist - [x] Read `CONTRIBUTING.md` - [x] Ran tests (`go test ./... -race`) locally - [x] Formatted and linted with `gofmt -w .` and `golangci-lint run ./...` respectively - [x] Added tests if necessary (significant changes/complex logic) - [x] Updated documentation if applicable - [x] Updated `README.md` if applicable ## Summary

claude[bot] (Migrated from github.com) reviewed

2026-04-15 22:31:46 -05:00

internal/tui/update_confirmations.go

The tea.Cmd returned by removeCurrentProject() is silently dropped here.

When the last project is deleted, removeCurrentProject() calls createDefaultProject(), which calls m.addNotification(...) and returns its tea.Cmd for auto-dismiss. That cmd propagates back through removeCurrentProject() — but is discarded at this call site.

The result: the "Created default project" notification will appear but will never auto-dismiss, which is exactly the bug this PR is supposed to fix.

	notifCmd := m.removeCurrentProject()
	m.UIState.Mode = state.NormalMode
	return m, tea.Batch(notifCmd, m.addNotification(state.LevelInfo, fmt.Sprintf("Project '%s' deleted", projectName)))

The `tea.Cmd` returned by `removeCurrentProject()` is silently dropped here. When the last project is deleted, `removeCurrentProject()` calls `createDefaultProject()`, which calls `m.addNotification(...)` and returns its `tea.Cmd` for auto-dismiss. That cmd propagates back through `removeCurrentProject()` — but is discarded at this call site. The result: the "Created default project" notification will appear but will never auto-dismiss, which is exactly the bug this PR is supposed to fix. ```suggestion notifCmd := m.removeCurrentProject() m.UIState.Mode = state.NormalMode return m, tea.Batch(notifCmd, m.addNotification(state.LevelInfo, fmt.Sprintf("Project '%s' deleted", projectName))) ```

claude[bot] (Migrated from github.com) reviewed

2026-04-15 22:31:54 -05:00

internal/tui/model.go

This deprecated wrapper explicitly discards the tea.Cmd from initEstimateInput. If any caller still uses initEstimateInputForForm and initEstimateInput returns an error (e.g., "No task selected"), the notification's auto-dismiss command is dropped — the notification won't go away.

If this function is still called anywhere (I see it's marked deprecated but not removed), it reintroduces the bug this PR fixes. Either remove the function, or propagate the cmd:

	_, _ = m.initEstimateInput(state.TicketFormMode)

If callers have already been migrated, remove this wrapper entirely rather than leaving a broken stub.

This deprecated wrapper explicitly discards the `tea.Cmd` from `initEstimateInput`. If any caller still uses `initEstimateInputForForm` and `initEstimateInput` returns an error (e.g., "No task selected"), the notification's auto-dismiss command is dropped — the notification won't go away. If this function is still called anywhere (I see it's marked deprecated but not removed), it reintroduces the bug this PR fixes. Either remove the function, or propagate the cmd: ```suggestion _, _ = m.initEstimateInput(state.TicketFormMode) ``` If callers have already been migrated, remove this wrapper entirely rather than leaving a broken stub.

claude[bot] commented

2026-04-15 22:32:09 -05:00

(Migrated from github.com)

Review Summary

Two concrete bugs and one missing-test concern:

Bug 1 — confirmDeleteProject drops the auto-dismiss cmd from removeCurrentProject (see inline comment on update_confirmations.go:233).
When the last project is deleted, removeCurrentProject() → createDefaultProject() creates a "Created default project" notification and returns its tea.Cmd for auto-dismissal. That cmd is dropped at the call site, so the notification will stay on screen indefinitely — the same failure mode this PR is intended to fix.

Bug 2 — initEstimateInputForForm explicitly discards the auto-dismiss cmd (see inline comment on model.go:1206).
The deprecated wrapper silences the return with _, _. Any caller still using this function will lose the auto-dismiss command on error notifications from initEstimateInput. Either remove the wrapper (if all callers are migrated) or stop discarding the cmd.

Missing test coverage: The PR checklist explicitly marks both "Ran tests" and "Added tests if necessary" as unchecked. This is a 400+-line change that deletes an entire package (modelops) and rewires notification propagation across every update handler. No tests were added to cover the new auto-dismiss flow, and existing tests were not verified to still pass. At minimum, run the tests before merging.

Score: 5/10

## Review Summary Two concrete bugs and one missing-test concern: **Bug 1 — `confirmDeleteProject` drops the auto-dismiss cmd from `removeCurrentProject`** (see inline comment on `update_confirmations.go:233`). When the last project is deleted, `removeCurrentProject()` → `createDefaultProject()` creates a "Created default project" notification and returns its `tea.Cmd` for auto-dismissal. That cmd is dropped at the call site, so the notification will stay on screen indefinitely — the same failure mode this PR is intended to fix. **Bug 2 — `initEstimateInputForForm` explicitly discards the auto-dismiss cmd** (see inline comment on `model.go:1206`). The deprecated wrapper silences the return with `_, _`. Any caller still using this function will lose the auto-dismiss command on error notifications from `initEstimateInput`. Either remove the wrapper (if all callers are migrated) or stop discarding the cmd. **Missing test coverage**: The PR checklist explicitly marks both "Ran tests" and "Added tests if necessary" as unchecked. This is a 400+-line change that deletes an entire package (`modelops`) and rewires notification propagation across every update handler. No tests were added to cover the new auto-dismiss flow, and existing tests were not verified to still pass. At minimum, run the tests before merging. **Score: 5/10**

👎 1