Feature 48/notification toasting layer #105

noe.trevino · 2026-02-18T02:27:02-06:00

noe.trevino commented

2026-02-18 02:27:02 -06:00

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

New notifications

## 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  New notifications <img width="3799" height="615" alt="image" src="https://github.com/user-attachments/assets/3667d62c-99e5-45cc-83e1-3f29011dea8e" />

github-actions[bot] commented

2026-02-18 02:27:12 -06:00

(Migrated from github.com)

Informational Warning:

The snapshot tests have been changed.
Please add a screenshot showing what has been changed, and a description of why it was changed.

If you do not provide an explanation and screenshot in 3 days, this PR will be closed.

@TheNoeTrevino

Changed snapshot files (2 files)

internal/testing/snapshots/tui/testdata/notification_error.golden
internal/testing/snapshots/tui/testdata/notification_warning.golden

**Informational Warning:** The snapshot tests have been changed. Please add a screenshot showing what has been changed, and a description of why it was changed. If you do not provide an explanation and screenshot in 3 days, this PR will be closed. @TheNoeTrevino <details> <summary>Changed snapshot files (2 files)</summary> - `internal/testing/snapshots/tui/testdata/notification_error.golden` - `internal/testing/snapshots/tui/testdata/notification_warning.golden` </details>

👍 1

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

2026-02-18 02:29:03 -06:00

internal/tui/components/column.go

					
				@ -97,6 +97,10 @@ func renderColumnWithTasksContent(

					availableHeight := height - columnOverhead

					maxVisibleTasks := max(availableHeight/TaskCardHeight, 1)

					// Clamp scrollOffset to prevent slicing past the end of the task list.

The clamp min(scrollOffset, len(tasks)) is off by one. When scrollOffset == len(tasks) and len(tasks) > 0, tasks[scrollOffset:endIdx] is empty but the "▲ more above" indicator still renders (because scrollOffset > 0), showing a misleading indicator with no tasks visible.

	scrollOffset = min(scrollOffset, max(len(tasks)-1, 0))

The clamp `min(scrollOffset, len(tasks))` is off by one. When `scrollOffset == len(tasks)` and `len(tasks) > 0`, `tasks[scrollOffset:endIdx]` is empty but the "▲ more above" indicator still renders (because `scrollOffset > 0`), showing a misleading indicator with no tasks visible. ```suggestion scrollOffset = min(scrollOffset, max(len(tasks)-1, 0)) ```

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

2026-02-18 02:29:09 -06:00

internal/tui/state/notification_state.go

CreatedAt is set here but never read anywhere in the codebase. Expiry is handled entirely by the tea.Tick timer in addNotification. This is dead code and should be removed from the struct.

`CreatedAt` is set here but never read anywhere in the codebase. Expiry is handled entirely by the `tea.Tick` timer in `addNotification`. This is dead code and should be removed from the struct.

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

2026-02-18 02:29:16 -06:00

internal/tui/update_normal.go

				`@ -165,8 +162,7 @@ func (m Model) handleNavigateLeft() (tea.Model, tea.Cmd) {`

Rapid key-repeat at a boundary (e.g., holding left arrow at column 0) now fires addNotification on every key event, spawning a new tea.Tick timer each time and pushing duplicate notifications into the stack (capped at 3, then evicting). The previous Clear() on each keypress made this idempotent. Consider deduplicating: skip adding if the top notification has the same message, or check before calling addNotification.

Rapid key-repeat at a boundary (e.g., holding left arrow at column 0) now fires `addNotification` on every key event, spawning a new `tea.Tick` timer each time and pushing duplicate notifications into the stack (capped at 3, then evicting). The previous `Clear()` on each keypress made this idempotent. Consider deduplicating: skip adding if the top notification has the same message, or check before calling `addNotification`.

claude[bot] commented

2026-02-18 02:29:45 -06:00

(Migrated from github.com)

Review Summary

3 issues found.

1. Off-by-one in scroll clamp (column.go line 100)
min(scrollOffset, len(tasks)) permits scrollOffset == len(tasks), which renders an empty task area while still showing the scroll-up indicator. The bound should be max(len(tasks)-1, 0).

2. Duplicate notification spam on held keys (update_normal.go boundary guards)
All boundary navigation handlers now call addNotification on every keypress. Holding a key at a boundary fires repeated identical notifications and timers, cycling through the 3-notification cap with eviction. The old Clear() per-keypress prevented this. A dedup check (skip if top notification has the same message) would restore the correct behavior.

3. Dead CreatedAt field (notification_state.go)
Notification.CreatedAt is assigned but never read. Expiry is handled by the tea.Tick timer, making this field unused dead code.

Score: 7/10

## Review Summary 3 issues found. **1. Off-by-one in scroll clamp** (column.go line 100) min(scrollOffset, len(tasks)) permits scrollOffset == len(tasks), which renders an empty task area while still showing the scroll-up indicator. The bound should be max(len(tasks)-1, 0). **2. Duplicate notification spam on held keys** (update_normal.go boundary guards) All boundary navigation handlers now call addNotification on every keypress. Holding a key at a boundary fires repeated identical notifications and timers, cycling through the 3-notification cap with eviction. The old Clear() per-keypress prevented this. A dedup check (skip if top notification has the same message) would restore the correct behavior. **3. Dead CreatedAt field** (notification_state.go) Notification.CreatedAt is assigned but never read. Expiry is handled by the tea.Tick timer, making this field unused dead code. --- **Score: 7/10**

Sign in to join this conversation.

No reviewers