Feature 145/improve task sidebar #147

noe.trevino · 2026-04-15T23:31:39-05:00

noe.trevino commented

2026-04-15 23:31:39 -05: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

## 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 23:36:38 -05:00

internal/tui/components/activity_preview.go

					
				@ -0,0 +1,102 @@

				package components

The empty-items branch is dead code. Both callers pre-guard before calling this function:

renderDetailComments returns early when len(comments) == 0 (before it ever builds items)
renderFormCommentsPreview handles activityCount == 0 with its own message before calling RenderActivityPreviews

Additionally, the label "No comments" is wrong for a function whose signature accepts generic ActivityItem values — it will also receive Events. Either remove the branch (since it's unreachable), or if the intent is to keep it for future callers, rename the message to "No activity".

The empty-items branch is dead code. Both callers pre-guard before calling this function: - `renderDetailComments` returns early when `len(comments) == 0` (before it ever builds `items`) - `renderFormCommentsPreview` handles `activityCount == 0` with its own message before calling `RenderActivityPreviews` Additionally, the label `"No comments"` is wrong for a function whose signature accepts generic `ActivityItem` values — it will also receive Events. Either remove the branch (since it's unreachable), or if the intent is to keep it for future callers, rename the message to `"No activity"`.

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

2026-04-15 23:36:44 -05:00

internal/tui/components/detail_panel.go

strings.Builder adds no value here. This function performs at most ~10 small string concatenations on already-rendered styled strings — not a hot loop over large data. The original header := ticketStyle.Render(ticketNumber) + header += "\n" + ... idiom was simpler and equally correct. strings.Builder is an optimisation for many allocations in a tight loop; using it here just adds noise.

`strings.Builder` adds no value here. This function performs at most ~10 small string concatenations on already-rendered styled strings — not a hot loop over large data. The original `header := ticketStyle.Render(ticketNumber)` + `header += "\n" + ...` idiom was simpler and equally correct. `strings.Builder` is an optimisation for many allocations in a tight loop; using it here just adds noise.

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

2026-04-15 23:36:49 -05:00

internal/tui/components/detail_panel_test.go

The test name CommentsAdaptToHeight implies the height-adaptive truncation behaviour is verified, but the test only checks the full-height case. There is no assertion that reducing the available height causes items to be hidden. A complementary call with a constrained height (e.g. height=10, which should squeeze out Bob and Charlie) would actually exercise the adaptation path introduced in this PR.

The test name `CommentsAdaptToHeight` implies the height-adaptive truncation behaviour is verified, but the test only checks the full-height case. There is no assertion that reducing the available height causes items to be hidden. A complementary call with a constrained height (e.g. `height=10`, which should squeeze out Bob and Charlie) would actually exercise the adaptation path introduced in this PR.

claude[bot] commented

2026-04-15 23:36:58 -05:00

(Migrated from github.com)

Overall: The DRY refactor (extracting RenderActivityBadge/RenderActivityPreviewItem/RenderActivityPreviews and deleting renderActivityBadgePreview) is correct and the height-aware comment rendering is a genuine improvement. Three issues:

Dead code with wrong label (activity_preview.go:93-97) — the empty-items branch in RenderActivityPreviews is unreachable from every current call site, and its "No comments" label is semantically wrong for a function that also handles Events.
Gratuitous strings.Builder (detail_panel.go:129) — introduced in renderDetailHeader for a handful of small concatenations; adds complexity with zero benefit over the original + idiom.
Test doesn't cover its stated purpose (detail_panel_test.go:570) — TestRenderDetailPanel_CommentsAdaptToHeight only asserts the full-height path; no assertion that a constrained height actually truncates visible items.

7/10

**Overall:** The DRY refactor (extracting `RenderActivityBadge`/`RenderActivityPreviewItem`/`RenderActivityPreviews` and deleting `renderActivityBadgePreview`) is correct and the height-aware comment rendering is a genuine improvement. Three issues: 1. **Dead code with wrong label** (`activity_preview.go:93-97`) — the empty-items branch in `RenderActivityPreviews` is unreachable from every current call site, and its `"No comments"` label is semantically wrong for a function that also handles Events. 2. **Gratuitous `strings.Builder`** (`detail_panel.go:129`) — introduced in `renderDetailHeader` for a handful of small concatenations; adds complexity with zero benefit over the original `+` idiom. 3. **Test doesn't cover its stated purpose** (`detail_panel_test.go:570`) — `TestRenderDetailPanel_CommentsAdaptToHeight` only asserts the full-height path; no assertion that a constrained height actually truncates visible items. **7/10**

👍 1

noe.trevino reviewed

2026-04-15 23:40:41 -05:00

internal/tui/components/detail_panel.go