feat: implement #329 — feat(cli): Issues tab — show GitHub number, author, date, PR created, humanize action

[sergiotejon]

Auto-generated by Heimdallm in response to #329.

Closes #329

[vbuenog]

🤖 Heimdallm AI Review

Adds useful filtering and richer issue display to the Issues tab. Main concern: the new sort-by-review-date will panic when LatestReview is nil, and there's minor width/truncation cosmetics.

Issues

🔴 cli/internal/tui/dashboard.go:243 — sort.Slice dereferences d.issues[i].LatestReview.CreatedAt unconditionally, but renderIssues and humanizeIssueAction both treat LatestReview as nillable (if iss.LatestReview != nil). If any issue has no review, this sort will nil-pointer panic. Guard with a nil check (e.g., sort by zero-time when LatestReview == nil, or skip such items).
⚠️ cli/internal/tui/dashboard.go:765 — num is built as fmt.Sprintf("#%d", iss.Number) then " D" appended for dismissed issues, then truncated to 7. For 5+ digit issue numbers the dismissed " D" marker is silently truncated/replaced by an ellipsis, hiding the dismissed state in the list. Either widen the column or place the dismissed marker in a dedicated column / separate symbol prefix.
🟡 cli/internal/tui/dashboard.go:746 — When issueActionCategory returns "---" (nil LatestReview), it becomes a selectable filter option via uniqueIssueActions; cycling to it filters by the literal string "---". That's probably unintentional — consider excluding issues without a review from the action-filter options, or labeling it clearly (e.g., "(no review)").
🟡 cli/internal/tui/dashboard.go:732 — Header uses widths 8/18/22/10/8/14/10 plus spaces (~94 cols + labels), but the separator is hardcoded to 106 dashes. When severity styling is applied the rendered severity cell is %-8s so header/row widths should match — verify and consider deriving the separator length from the header string instead of a magic number.
🟡 cli/internal/tui/dashboard.go:395 — cycleFilter uses strings.EqualFold for comparisons, but repo names on GitHub can be case-sensitive in theory and Heimdall stores them as-is. Using case-insensitive compare on visibleIssues filter (EqualFold) could coalesce distinct repos. Low risk in practice but prefer exact compare for repo.
🟡 cli/internal/tui/dashboard.go:387 — On 'enter' with tabIssues, bounds check is against len(d.visibleIssues()) (recomputed), and openDetail() recomputes it again. Minor perf/consistency nit — compute once and pass, or cache the filtered slice on the Dashboard when filters change.

Suggestions

• Make the sort nil-safe: e.g., pick zero time when LatestReview is nil so sortable items float to the bottom instead of crashing.
• Add a small unit test for visibleIssues / cycleFilter / humanizeIssueAction — they're pure functions and would catch regressions (empty options, single option, wrap-around to '').
• Consider caching visibleIssues() result on the Dashboard whenever issues/filters change, rather than recomputing in openDetail, renderIssues, and tabItemCount.
• Dismissed issues could be styled (e.g., muted/strikethrough row) instead of relying on a ' D' suffix that gets truncated for longer numbers.
• Document the new f/F key bindings in README/help docs if there's a keybinding reference outside the TUI help line.

---

Severity: HIGH · Reviewed by Heimdallm

[Muriano]

🔴 HIGH Issue

Nil pointer dereference in sort comparator: d.issues[i].LatestReview.CreatedAt is accessed without nil-checking LatestReview, which is *IssueReview (can be nil per the struct definition and confirmed by nil guards elsewhere in the same file, e.g., if iss.LatestReview != nil in renderIssues). If any issue has no review yet, this will panic at runtime. Fix: add nil guards in the sort less-func, e.g., treat nil LatestReview as zero time so unreviewed issues sort to the end.

Location: cli/internal/tui/dashboard.go line 243

---

Posted by Heimdallm AI Review

[Muriano]

⚠️ MEDIUM Issue

visibleIssues() rebuilds a filtered slice on every call and is invoked multiple times per render cycle (renderIssues, tabItemCount, handleKey enter, openDetail). While not a correctness bug for small lists, it also means the filtered result can theoretically differ between calls within the same Update/View if d.issues were mutated concurrently. Consider caching the result once per render pass or storing it as a field invalidated on filter/data change.

Location: cli/internal/tui/dashboard.go line 715

---

Posted by Heimdallm AI Review

[Muriano]

🤖 Heimdallm AI Review — Summary

Good feature PR adding GitHub number, author, date, labels, humanized actions, and filtering to the Issues tab. One confirmed crash bug in the sort comparator and a few minor suggestions.

2 issue(s) found — see individual comments above for details.

Suggestions

• In cycleFilter, consider using strings.EqualFold consistently — currently it uses EqualFold for comparison but the returned value preserves original casing from options[], which is fine, but worth noting that the filter matching in visibleIssues also uses EqualFold, so this is consistent.
• The column widths in renderIssues header (%-8s %-18s %-22s etc.) total ~106 chars and are hardcoded. On narrow terminals this will wrap or truncate poorly. Consider adapting column widths to d.width or at minimum documenting the minimum supported width.
• The help text for the Issues tab no longer mentions [pgup/pgdn]page, while the PRs tab still does. If page up/down still works on Issues, it should be documented; if it doesn't, that's a minor UX regression.

---

Severity: HIGH · Reviewed by Heimdallm

[ivanmunozruiz]

🤖 Heimdallm AI Review

The PR adds useful filtering, sorting, and richer columns to the Issues tab, but introduces a HIGH-severity nil pointer panic in the new sort comparator and a few smaller cursor/edge-case concerns around the new filter state.

Issues

🔴 cli/internal/tui/dashboard.go:243 — Nil pointer dereference: sort.Slice uses d.issues[i].LatestReview.CreatedAt with no nil guard. LatestReview is *IssueReview and can legitimately be nil (confirmed by the nil check in renderIssues). The first fetch where any issue lacks a review will panic the TUI. Guard with something like: iCreated := time.Time{}; if d.issues[i].LatestReview != nil { iCreated = d.issues[i].LatestReview.CreatedAt } (and same for j), so unreviewed issues sort to the end deterministically.
⚠️ cli/internal/tui/dashboard.go:389 — Enter handler on tabIssues uses d.cursor < len(d.visibleIssues()) but openDetail() re-derives vis and also guards with d.cursor < len(vis); the extra guard is fine, yet if the user changes filter mid-flight elsewhere without resetting d.cursor (e.g. after a refresh that shrinks visible set), d.cursor may still point out of range on next key. cycleFilter handlers reset cursor, but the r-refresh path does not clamp d.cursor against the new filtered length — consider clamping d.cursor in the tea.Msg branch after issues are re-sorted/filtered to keep selection consistent.
🟡 cli/internal/tui/dashboard.go:1240 — cycleFilter silently returns "" (clears the filter) when current is non-empty but not found in options. If a refresh removes the last issue matching the active filter, the user's filter silently disappears without a visible signal — and because uniqueIssueActions/Repos are derived from d.issues, the option literally vanishes. Consider preserving the filter string and letting visibleIssues() simply return empty (the existing empty-state message already handles that), or surface a 'filter no longer applies' hint.
🟡 cli/internal/tui/dashboard.go:760 — num column is truncated to 7 runes after appending the ' D' dismissed suffix. For issues with 5+ digit numbers (e.g. '#12345 D' = 8 chars) the 'D' flag is lost to the ellipsis, so a dismissed issue is indistinguishable from an active one in the list view. Either widen the column, render the D flag as a separate styled column, or truncate the number before appending the suffix.
🟡 cli/internal/tui/dashboard.go:715 — visibleIssues() is called several times per render (renderIssues, tabItemCount, openDetail, handleKey). For typical issue counts this is fine, but caching the slice once per Update/View pass (or invalidating on filter/data change) would avoid repeated allocations and guarantee a consistent view within a single frame.
🟡 cli/internal/tui/dashboard.go:1154 — humanizeIssueAction and issueActionCategory duplicate the same switch on ActionTaken with near-identical branches. Because the filter compares issueActionCategory ("Implemented") against the unhumanized value while the list column shows humanizeIssueAction ("→ PR #123"), users filtering by 'Implemented' still see '→ PR #...' rows, which is correct but worth a short comment explaining the split so future edits don't accidentally unify them and break filtering.
🟡 cli/internal/tui/dashboard.go:935 — Help text for the Issues tab drops [pgup/pgdn]page, but the underlying key handlers for pgup/pgdn are almost certainly still active (shared across tabs). Either document them for consistency with the PRs tab or explicitly disable them on Issues — the current state hides a working feature from users.

Suggestions

• Add a unit test covering sort ordering when some issues have LatestReview == nil to lock in the fix and prevent regressions.
• Consider extracting the Issues column widths into named constants — the magic numbers 7/16/20/9/13/14 in truncateRunes calls must stay in sync with the %-Ns widths in both header and line format strings, which is fragile.
• parseIssueLabels swallows json.Unmarshal errors silently; at minimum log at debug level so malformed label payloads don't disappear without a trace.
• When the filter line is shown, overhead is incremented but the scroll indicator height math still assumes height-overhead; double-check maxVisible math on short terminals (e.g., height=3 with a filter line renders maxVisible=0 clamped to 1, which may overlap the scroll indicator).

---

Severity: HIGH · Reviewed by Heimdallm

[Muriano]

🤖 Heimdallm AI Review

The HIGH nil-pointer issue from the previous review has been properly fixed with nil guards in the sort comparator. The MEDIUM visibleIssues() caching concern remains unaddressed but is not a correctness bug given Bubble Tea's single-threaded model. No new issues introduced.

Suggestions

• visibleIssues() is still rebuilt on every call and invoked multiple times per render (renderIssues, tabItemCount, handleKey, openDetail). Since Bubble Tea is single-threaded this is safe, but caching the slice once per Update/View pass would avoid redundant allocations as the issue list grows.

---

Severity: LOW · Reviewed by Heimdallm

[ivanmunozruiz]

🤖 Heimdallm AI Review

Re-review: the HIGH nil-pointer dereference in the sort comparator has been addressed with explicit nil guards that push unreviewed issues to the end. The remaining findings from the prior review are unchanged in this diff and still warrant attention, though none are blockers on their own.

Issues

⚠️ cli/internal/tui/dashboard.go:250 — Cursor is not clamped to len(visibleIssues()) after a refresh (tea.Msg branch). cycleFilter handlers reset d.cursor to 0, but the 'r' refresh path does not, so if the background fetch returns fewer issues matching the active filter, d.cursor can point past the end of the filtered slice. openDetail() guards with 'if d.cursor < len(vis)' (so no panic), but rendering/selection still points at a ghost row until the user moves. Clamp d.cursor against len(d.visibleIssues()) in the tea.Msg branch after the sort.
🟡 cli/internal/tui/dashboard.go:1258 — cycleFilter still silently returns "" when the current filter value is not found in the (freshly derived) options list. Because uniqueIssueRepos/uniqueIssueActions are derived from d.issues, a refresh that removes the last matching issue will make the active filter vanish with no user-visible signal. Preferable: keep the filter string (visibleIssues() will just return empty and the existing empty-state message kicks in), or surface a 'filter no longer matches' hint.
🟡 cli/internal/tui/dashboard.go:778 — num column is still truncated to 7 runes after the ' D' suffix is appended, so for issue numbers with 5+ digits (e.g. '#12345 D' = 8 chars) the dismissed flag is eaten by the ellipsis and a dismissed issue becomes visually identical to an active one. Truncate the number before appending the suffix, widen the column, or render D as a separate styled column.
🟡 cli/internal/tui/dashboard.go:1220 — visibleIssues() is still called several times per render (renderIssues, tabItemCount, handleKey, openDetail) and re-allocates the filtered slice each time. Fine for current scale but worth caching once per Update/View pass (or as a field invalidated on filter/data change) for consistency and to avoid repeat allocations.
🟡 cli/internal/tui/dashboard.go:1175 — humanizeIssueAction and issueActionCategory still duplicate the switch on ActionTaken with near-identical branches, and the split is load-bearing: the list column renders the humanized value ('→ PR #123') while the filter compares against the category ('Implemented'). A future refactor that unifies them would silently break filtering. Add a short comment on both functions explaining why they must stay separate.
🟡 cli/internal/tui/dashboard.go:943 — Issues-tab help text still drops [pgup/pgdn]page, but the shared key handlers for pgup/pgdn remain active on this tab. Either re-add the hint for parity with the PRs tab or explicitly disable those keys on Issues — current state hides a working feature.

Suggestions

• Consider clamping d.cursor against len(d.visibleIssues()) in the fetch-result branch where d.issues is re-sorted, to keep selection consistent across refreshes that shrink the filtered set.
• Add a short doc comment on humanizeIssueAction and issueActionCategory noting that the split is intentional (display vs filter key) so they aren't merged later.

---

Severity: MEDIUM · Reviewed by Heimdallm

[vbuenog]

🤖 Heimdallm AI Review

The HIGH nil-pointer panic in the sort comparator has been addressed with proper nil guards. Remaining concerns are unchanged from previous review: dismissed marker truncation for large issue numbers, '---' as filter option, hardcoded separator width, and repeated visibleIssues() recomputation.

Issues

⚠️ cli/internal/tui/dashboard.go:768 — num column still truncated to 7 chars. For 5+ digit issue numbers, '#12345' alone is 6 chars; appending ' D' yields 8 chars which gets truncated to '#12345…', silently hiding the Dismissed marker. Either widen the column or use a separate marker column/symbol prefix (e.g., a glyph before the number).
🟡 cli/internal/tui/dashboard.go:1243 — issueActionCategory returns '---' for issues without LatestReview, and uniqueIssueActions includes this as a cycleable filter option. Cycling to '---' then filters by the literal string. Either exclude unreviewed issues from the action options or label them clearly (e.g., '(no review)') and handle that case explicitly.
🟡 cli/internal/tui/dashboard.go:738 — Header separator is hardcoded to 106 dashes while column widths sum to ~94 + label column. Derive the separator length from the header string (utf8.RuneCountInString) rather than a magic number to avoid drift when columns are tweaked.
🟡 cli/internal/tui/dashboard.go:1213 — visibleIssues() rebuilds the filtered slice on every call (renderIssues, tabItemCount, handleKey enter, openDetail). Cache it as a field invalidated when filters/data change, to both avoid redundant work and guarantee consistency within a single Update/View pass.
🟡 cli/internal/tui/dashboard.go:1233 — cycleFilter uses strings.EqualFold when comparing repos. GitHub repo names preserve case, and Heimdall stores them as-is; exact compare is safer to avoid coalescing distinct repos that only differ in case.

Suggestions

• Nil-guard fix in sort comparator looks correct; consider also adding a stable secondary sort key (e.g., Issue.Number) so ordering is deterministic when LatestReview timestamps tie or are both nil.
• Consider a dedicated 1-char DISMISSED column (e.g., 'D' or '✓') rather than appending ' D' to the number, which survives truncation regardless of issue number length.
• Add a small unit test for cycleFilter covering empty options, wrap-around to '', and unknown current values — easy to cover and protects against regressions.

---

Severity: MEDIUM · Reviewed by Heimdallm

[vbuenog]

🤖 Heimdallm AI Review

Re-review: the sort.Slice nil-guard (previous HIGH) is now properly handled with explicit nil checks for LatestReview. Remaining concerns from the prior review are still present in the unchanged portions of the code.

Issues

⚠️ cli/internal/tui/dashboard.go:765 — Dismissed marker still silently truncated for 5+ digit issue numbers: num = "#NNNNN" + " D" exceeds the 7-rune truncation, so the " D" marker is replaced by an ellipsis and the dismissed state disappears from the list. Widen the column or move the dismissed indicator to a dedicated column / distinct prefix glyph.
🟡 cli/internal/tui/dashboard.go:746 — issueActionCategory returns "---" for issues with nil LatestReview, and uniqueIssueActions includes it in the cycle. Cycling to "---" filters by the literal string, which is surprising UX. Either exclude nil-review issues from the action filter options or label it clearly (e.g., "(no review)").
🟡 cli/internal/tui/dashboard.go:732 — Separator length is hardcoded to 106 dashes while column widths come from the header format. Derive the separator width from the header string to avoid drift when columns are tweaked.
🟡 cli/internal/tui/dashboard.go:395 — cycleFilter uses strings.EqualFold for repo comparison; repo names are case-sensitive on GitHub in theory. Prefer exact compare for repos to avoid coalescing distinct repos.
🟡 cli/internal/tui/dashboard.go:715 — visibleIssues() is rebuilt on every call (renderIssues, tabItemCount, handleKey enter, openDetail) — at least 3–4 times per render cycle. Cache once per render/update or store on the Dashboard invalidated when filters or d.issues change. Also reduces risk of divergent filtered views within a single Update/View pass.

Suggestions

• Good fix on the sort comparator — nil LatestReview entries now sort to the end safely.
• Consider replacing the 106-dash magic separator with lipgloss.NewStyle().Width(...) or strings.Repeat("─", lipgloss.Width(header)) to auto-track column widths.
• If the dismissed marker is important signal, render it as a status glyph (e.g., leading ✗ or strike-through style) rather than a trailing " D" inside the # column.

---

Severity: MEDIUM · Reviewed by Heimdallm