Repository navigation
v1.9 tranche C: report dashboard — table, sparkline, gauges (#117, #66, #80) - #139
Merged
Merged
Conversation
Fixes the review round-1 defect (#66): truncate() cuts labels by rune count, but reportItemWidth and lipgloss/table's own resizer both size columns by display width, so a label with double-width runes (CJK, emoji) could truncate to itemW runes and still render up to 2x that many columns wide, blowing the terminal budget. reportTable now shaves runes one at a time after truncating until the rendered width actually fits, and TestReportTableNeverExceedsWidth gained an all-emoji label case to guard it. Also, from the same review round: - report.go: reportModel.view no longer renders the table and then discards it for an empty report; it's a real if/else now. - report_test.go: added a test pinning the empty-report view (the "No hours to show." box plus the billable split line below it). - report_table_test.go: corrected two misleading test comments (Wrap(false) is insurance, not what TestReportTableTruncatesRather- Wraps actually pins; TestReportTableFloorsRatherThanVanishing exercises the label-cap branch, not the 12-column floor), added an assertion that actually pins the floor, and added an assertion that per-currency subtotal rows are not bold. - golden_test.go: fixed the multi-currency golden fixture so BillableHours grows with the added Mobile bucket instead of leaving a TOTAL row that doesn't add up to the billable/non-billable split; regenerated report_multicurrency.golden.
… guard against 24-hour arithmetic
…view - rates_test.go: TestRatesSelectionIsPerSection was inserted directly under TestRatesDraftRateStepKeyLabels's doc comment with no blank line, so the comment documented the wrong test and the other lost its own. Re-attach each comment to its own function. - theme.go: Cell's doc comment said "colour"; American spelling is the repo convention.
GroupByTotal's single bucket carries every currency in the workspace, and
it's the default first-load grouping. formatAmounts joins them onto one
line ("12345.00 EUR + 6789.00 USD + ..."), reportItemWidth's Item-column
budget went deeply negative computing against that unbounded width, Item
clamped to its floor, and the Amount column rendered straight past the
terminal edge: 3 currencies overflowed at 60 columns, 5 currencies even at
100.
Item still yields first — it's the point of having a grid. Only once Item
is already at its floor and the row would still overflow does the new
reportAmountWidth claw space back from Amount, truncating it the same way
Item already does (by display width, via the shared shaveToWidth helper,
not rune count — the third time on this branch that mismatch has been the
bug). This is safe because a per-bucket Amount is indicative only:
CurrencySubtotals is the authoritative, full-precision figure and renders
a few rows down, untruncated.
Piggybacking on this file:
- reportItemWidth no longer takes a headers parameter now that no caller
varies it; reads the package-level reportHeaders directly.
- formatAmounts moves next to its only caller, reportRows, now that the
table lives in its own file.
- truncate now guards n <= 1 (returning the ellipsis outright) instead of
relying on every call site keeping n above the panic threshold — the
shave loop is safe today only because reportItemWidth can never return
less than 4, and reportAmountWidth has no equivalent floor.
TestReportTableNeverExceedsWidth gains a 3-currency and a 5-currency
bucket and keeps its absolute assertion (no width was carved out for
"amounts still overflow, just less").
DailyHours zero-fills the whole range and a zero renders as a space, and the label was appended after the chart, so for the current month viewed partway through — the default view — the label drifted as many columns as there were trailing idle days (10 bars, 21 blanks, then "hours/day" adrift 21 columns to the right). Visible in the shipped docs/demo.gif. sparkLabel is now a prefix, so the chart always reads left-to-right from a fixed origin and the label can't drift. The chart's trailing spaces are trimmed before assembling the line — trailing idle days carry no ink either way, so nothing visible changes, but it keeps goldens free of trailing whitespace an editor could silently strip. A zero still renders as a space, not the lowest bar: that reads as "a little work" where there was none, per sparkline.go's own comment, which stays true. maxCells now measures the label with lipgloss.Width instead of len — it's ASCII today so the value doesn't change, but this is the file that already learned twice that byte counts aren't display columns. Adds a golden and a discriminating unit test whose daily series ends in zeros — the shape of the default view that goldenDaily() never covered, since its own series happens to end non-zero.
…kline Neither README mentioned that the report now sizes itself to the terminal (the label column giving up space first) or the per-day sparkline above it, while both already document the budget burn-down in similar detail.
reportAmountWidth's comment claimed the authoritative per-currency subtotal
rows are "never truncated" because their labels are short — false below
~52 columns, since amountW is one column width shared by every row,
subtotals included. State the actual bound instead: subtotals fit whole
at 60 columns and above (the table's tested design range), and below the
~48-column point where Item already hits its floor the whole table is in
its documented degraded regime.
Also fix README.it.md's "uno sparkline giornaliero" to the correct
feminine gender ("una sparkline giornaliera"), by analogy with "la linea".
shaveToWidth's comment claimed safety "for any w, including w <= 1" but
w <= 0 is an infinite loop: truncate(s, 0) returns "…" (width 1), which
never satisfies "<= 0" so the shave-one-rune loop never terminates. Add
an explicit early return for w <= 0 and state the honest bound (safe for
w >= 1); unreachable today (itemW >= 4, amountW >= 1), but a hang gives
no diagnosis where a documented precondition does.
truncate("", n) returned "…" for n <= 1, turning the multi-currency
TOTAL row's deliberately empty Amount cell into a stray ellipsis on
narrow terminals. An empty input now always yields "".
sparkView trimmed only the chart's trailing spaces, missing the label's
own trailing separator space: an all-zero daily series renders a fully
blank chart, so the assembled line ("hours/day " with nothing after)
still carried the exact trailing whitespace the trim was meant to
prevent. Trim the assembled label+chart instead. The line still renders
in that case — a chart with no ink is itself the information, the same
way one idle day among several renders as a blank cell rather than
vanishing.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fourth tranche of the v1.9 TUI design system milestone. Turns the hours report into a dashboard and clears the tranche's blocking tech debt.
Closes #117. Closes #66. Closes #80.
What changes for a user
Amountwas never aligned before), rows alternate a subtle background, and the totals block sits inside the same grid so everything lines up. The label column gives up space first; on a wide terminal it stops growing at its content instead of stretching. Before, the layout was a fixed 32-columnfmt.Sprintfthat wrapped on a narrow terminal and wasted space on a wide one.▁would read as "a little work" where there was none.Notable decisions
bubbles/progresswas evaluated and rejected for the gauge, despite #80 naming it. ItspercentageViewrunsmath.Max(0, math.Min(1, percent))before formatting, so a budget burned to 130% renders as100%— it hides the one fact that screen exists to show. It also readstermenv.ColorProfile()(the real terminal) rather than the injected renderer. The existing pure function already got the clamp asymmetry right and only needed better glyphs and color.table.Width()is deliberately never called. WithBorderColumn(false), lipgloss/table's resizer counts only the separators between columns and so believes the frame is free, whileString()clips withMaxWidth— the table would render two columns too wide and then have its right border sliced off. Sizing is implicit: pre-formatted numeric cells, labels pre-truncated,Padding(0, 1)from the style function.The
Amountcolumn can be truncated after all. The spec originally forbade it ("truncating an amount hides money") and that was wrong:GroupByTotalis the first-load grouping and its single bucket carries every currency in the workspace, so three currencies overflowed a 60-column terminal and five overflowed even at 100. A per-bucket amount is indicative —PerDayrounding drifts from the subtotals at fine groupings — whileCurrencySubtotalsis authoritative and renders as its own rows lower in the same table. Cutting an indicative figure does not hide money; the exact one is four rows down. The spec carries an amendment recording this.The sparkline's label leads rather than follows. With the label appended, a range ending in idle days pushed it 20+ columns adrift of the chart — visible in the README GIF. Leading the label anchors the chart to a fixed origin.
On the tests
Four tests in this branch were caught passing against the very bug they existed to catch, and all four were fixed:
Add(24 * time.Hour), because a spring-forward from midnight lands at 01:00 the same calendar day; the discriminating direction is the autumn fall-back, where +24h yields a duplicate day key and loses a day;Every new test for a specific bug was subsequently verified to fail against that bug before being accepted.
TestMainpinstermenv.Ascii, which strips backgrounds and bold, so the goldens are structurally blind to color — every color decision (zebra parity, the totals row, the over-budget fill) has a direct test asserting on the returnedlipgloss.Styleinstead.#117, for the record
The four parallel selection indices are now one
sel [secCount]int.move()loses both switches;selCount()keeps its own, because those four counts genuinely differ and that switch was never the duplication. A characterization test pinning per-section selection was written and shown to pass before the refactor — there is no RED step for a behavior-preserving change.Follow-ups filed
#135 (truncate by display width across the other five call sites), #136 (budget screen overflows 80 columns), #137 (the report repeats itself under the Total grouping), #138 (numeric columns reserved rather than clamped; two frame weights on one screen).
Verification
gofmt,go vet,staticcheck,go build,go test ./... -raceall clean on a purged test cache. Demo-mode parity confirmed by driving the TUI under a pty;docs/demo.gifregenerated and a frame inspected.🤖 Generated with Claude Code