Improve calendar view#70
Conversation
There was a problem hiding this comment.
Pull request overview
This PR upgrades the TUI calendar section from a simple recordings list to a richer, scrollable calendar UI with multiple display modes (Day/Week/Year) and user-aware week-start behavior.
Changes:
- Add new calendar renderers (day/week/year) and a viewport-backed calendar display with a
vkey toggle. - Fetch identity to honor
firstWeekDay, and expandRecordingmodel support for named-day labels (Label) and completion markers. - Tweak the loading hourglass animation timing/frames.
Tip
If you aren't ready for review, convert to a draft PR.
Click "Convert to draft" or run gh pr ready --undo.
Click "Ready for review" or run gh pr ready to reengage.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| internal/tui/loading.go | Adjusts hourglass flip timing and adds intermediate rotation frames. |
| internal/tui/content.go | Removes the old calendar recordings list UI implementation. |
| internal/tui/calendar.go | Switches calendar to viewport rendering, adds view mode toggling and identity-based first weekday. |
| internal/tui/calendar_views.go | Introduces the Day/Week/Year rendering pipeline and supporting helpers. |
| internal/tui/calendar_test.go | Updates tests for the new calendar state shape and view mode cycling. |
| internal/models/calendar.go | Adds Recording.Label to support named-day labels in the calendar UI. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if len(s) <= maxLen { | ||
| return s | ||
| } | ||
| if maxLen <= 1 { | ||
| return "…" | ||
| } | ||
| return s[:maxLen-1] + "…" | ||
| } | ||
|
|
||
| func centerPad(s string, width int) string { | ||
| pad := width - len(s) | ||
| if pad <= 0 { | ||
| return s[:width] |
There was a problem hiding this comment.
truncateStr truncates by byte length (len / s[:...]), which can split UTF-8 sequences and mis-measure terminal cell width (e.g., emoji/CJK), causing broken characters and misaligned layout. Consider truncating by runes or, better, by displayed width (e.g., using lipgloss.Width/runewidth) and ensuring you only cut at rune boundaries.
| if len(s) <= maxLen { | |
| return s | |
| } | |
| if maxLen <= 1 { | |
| return "…" | |
| } | |
| return s[:maxLen-1] + "…" | |
| } | |
| func centerPad(s string, width int) string { | |
| pad := width - len(s) | |
| if pad <= 0 { | |
| return s[:width] | |
| if lipgloss.Width(s) <= maxLen { | |
| return s | |
| } | |
| if maxLen <= 1 { | |
| return "…" | |
| } | |
| runes := []rune(s) | |
| for len(runes) > 0 && lipgloss.Width(string(runes))+lipgloss.Width("…") > maxLen { | |
| runes = runes[:len(runes)-1] | |
| } | |
| return string(runes) + "…" | |
| } | |
| func centerPad(s string, width int) string { | |
| pad := width - lipgloss.Width(s) | |
| if pad <= 0 { | |
| runes := []rune(s) | |
| for len(runes) > 0 && lipgloss.Width(string(runes)) > width { | |
| runes = runes[:len(runes)-1] | |
| } | |
| return string(runes) |
| func centerPad(s string, width int) string { | ||
| pad := width - len(s) | ||
| if pad <= 0 { | ||
| return s[:width] | ||
| } | ||
| left := pad / 2 | ||
| right := pad - left | ||
| return strings.Repeat(" ", left) + s + strings.Repeat(" ", right) | ||
| } |
There was a problem hiding this comment.
centerPad uses len(s) and slices s[:width], which is byte-based and can split multibyte UTF-8 characters; it also doesn't account for visual width (some runes take 2 cells). This can cause panics/garbled output and header misalignment; consider using rune-safe truncation and lipgloss.Width (or similar) when computing padding.
| { // 8: 90° horizontal — sand on right | ||
| "╭─ ", | ||
| "│ ──╮ ", | ||
| "╰─ ⣠╭── ", | ||
| " ──╯ ⣾⣿⣿─╮", | ||
| " ╰──⣿│", | ||
| " ─╯", | ||
| }, | ||
| { // 8: 90° horizontal — sand on right | ||
| " ", |
There was a problem hiding this comment.
Frame index comments in hourglassFrames are now inconsistent (multiple frames are labeled "8" after adding intermediate rotation frames). This makes it hard to reason about the animation and can lead to off-by-one mistakes when tweaking phases/indices; please renumber the frame comments to match the actual array indices (now 0–12).
| identity, err := v.vc.sdk.Identity().GetIdentity(v.vc.ctx) | ||
| if err != nil || identity == nil { | ||
| return identityLoadedMsg{firstWeekDay: time.Monday} | ||
| } | ||
| return identityLoadedMsg{firstWeekDay: time.Weekday(identity.FirstWeekDay)} | ||
| } |
There was a problem hiding this comment.
identity.FirstWeekDay is cast directly to time.Weekday without validation. If the API ever returns an out-of-range value, downstream code (week calculations, labels) can behave unpredictably. Consider clamping/normalizing to 0–6 (or defaulting to Monday) before storing it in v.firstWeekDay.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
4 issues found across 6 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="internal/tui/calendar.go">
<violation number="1" location="internal/tui/calendar.go:69">
P2: Initial recordings can be fetched with the wrong week-start because identity loading races with data fetch, and identity updates do not trigger a corrective refetch.</violation>
</file>
<file name="internal/tui/loading.go">
<violation number="1" location="internal/tui/loading.go:94">
P2: Frame index comments are stale/inconsistent after expanding to 13 frames; multiple rotation frames are mislabeled as `// 8`, making frame-to-index mapping misleading.</violation>
</file>
<file name="internal/tui/calendar_views.go">
<violation number="1" location="internal/tui/calendar_views.go:807">
P2: truncateStr slices by byte length, which can cut multibyte UTF‑8 runes and render garbled characters. Use rune-aware truncation so event titles with non‑ASCII characters stay valid.</violation>
<violation number="2" location="internal/tui/calendar_views.go:816">
P2: `centerPad` uses byte-based `len(s)` for width measurement and `s[:width]` for truncation, which can slice through multi-byte UTF-8 sequences, causing a panic or garbled output. Use `lipgloss.Width(s)` for visual width and rune-safe truncation.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| } | ||
|
|
||
| func (v *calendarView) Init() tea.Cmd { | ||
| cmds := []tea.Cmd{v.fetchIdentity()} |
There was a problem hiding this comment.
P2: Initial recordings can be fetched with the wrong week-start because identity loading races with data fetch, and identity updates do not trigger a corrective refetch.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At internal/tui/calendar.go, line 69:
<comment>Initial recordings can be fetched with the wrong week-start because identity loading races with data fetch, and identity updates do not trigger a corrective refetch.</comment>
<file context>
@@ -44,24 +58,32 @@ type calendarView struct {
}
func (v *calendarView) Init() tea.Cmd {
+ cmds := []tea.Cmd{v.fetchIdentity()}
if len(v.calendars) == 0 {
v.loading = true
</file context>
No description provided.