Repository navigation
v1.9 tranche D — layout & correctness - #145
Merged
Merged
Conversation
Closes-design-for: #137, #136, #135, #141, #138 (point 1), #28 (open boxes). Every measurement in the spec carries the fixture that produced it. Three issue claims turned out wrong and are corrected in place: #138's "5 unused columns" was right for its own long-label fixture (short labels leave 21+), its 84-in-80 overflow needs a long label AND numbers wider than 8, and #28's "demo is near-empty for non-month presets" was already fixed by #4.
Ten tasks: the two display-width helpers, the 15-call-site migration, the footer cut, the duplicate total row, the measured numeric columns, the width-aware budget screen, the pinned range, the Filters window, the tolerant task_tags decoder plus the micro-nits, and the changelog. Each task that closes a defect carries the RED it must be seen failing on. Task 3 (#141) is the one declared exception: the defect is behaviorally inert and invisible to the suite by construction, so its proof is the grep.
The review found six structural defects that would have broken the build or produced tests that cannot go green, all now fixed: - shaveToWidth has 5 callers, 3 of them in palette.go — deleting it as the plan described would not compile. Task 2 now migrates them. - Three report-table tests asserted widths below the minimum the fix can reach (48 long-label / 43 short-label), so they would have failed against correct code. Replaced with the measured sweep: 60/80/100 and 44. - The budget test asserted 80-column fitting at width 60, where the row overflows by design; the spec's "~48" threshold was the report table's, the budget's is 63-65. Both corrected. - newTestModel() takes no arguments, the Model field is filtersScreen, and newFilters derives its options from the entries — every Task 8 fixture was built on the wrong parameter, giving a cursor that could not move and a test that passed against any implementation. - The range-pin test depended on the machine's timezone: on UTC+2 both instants fall on the same local day, so it would pass in CI and fail locally. It now pins loc to UTC. - No golden exists under GroupByTotal, so Task 4's "watch the goldens fail" described something that does not exist; two goldens are created instead. Also corrected in the spec: the width-40 figure was 44 rendered, not 47 computed — reportAmountWidth claws space back from Amount, which hand arithmetic missed.
Every call site that cut or padded a string for layout used the rune-based truncate/shaveToWidth pair, which agrees with display columns only for ASCII. With CJK or emoji list and task names, a fixed-width row (rates list, entries browser, budget screen) could render wider than its own column. Migrate all 20 call sites to the display-width-aware helpers from Task 1 (truncateWidth for a bare cut, cell for a cut+pad pair), delete truncate and shaveToWidth, and rewrite the two doc comments that described the old guard semantics. Two tests exercised the doomed functions directly and needed the same migration to keep the package compiling.
Add TestReportRowsKeepsMultipleBucketsUnderTotalGrouping so the guard's len(r.Buckets) == 1 term is covered rather than only accidentally safe via internal/report's current aggregation. Fix reportRows' doc comment, which still claimed bucket rows always come first.
…hem (#138) reportNumWidth reserved a flat 8 columns for each of Hours and Billed, but nothing enforced that reservation matched what lipgloss/table actually renders: a value wider than 8 pushed the table past the terminal edge by exactly 2 x (width - 8). reportNumWidths measures Hours and Billed from the rows instead, headers included -- lipgloss/table sizes each column from every row it is given, the header row among them, so the measurement has to match that or the arithmetic still drifts by a smaller amount.
…#138) Two of the three tests added for #138 were missing t.Parallel(), the only tests in the file without it -- they were copied verbatim from the brief instead of following the file's convention. Two doc comments also asserted numbers that were never rendered: - the short-label/wide-numbers test claimed the fix renders 43 at width 44; rendering it shows 44 (Item capped at 7, Amount trimmed to 7 by the budget) -- 43 is the floor reached only below 44. - the header test claimed data-only measurement assumes 10 against the renderer's 11; for its actual row ("1.00" x2) data-only is 8, not 10, so the real overflow is 3, not 1.
… table (#136) TestBudgetBarLabelAgreesWithTheFill computed its expected full-block count with the same formula renderBudgetBar uses, so a bug in that formula would corrupt both sides identically and never fail. Swap it for a hand-measured table of pct -> wantFull literals.
A relative preset (last_7d, this_week) re-resolved time.Now() on every rebuild, so regrouping after midnight relabeled the report with a range the loaded entries never covered. entriesMsg/entriesReloadedMsg now carry the start/end the load itself resolved; the handlers pin them into Model.loadedStart/loadedEnd before rebuilding. activeRange() returns that pinned pair once a load has happened, else falls back to currentRange(). Every rebuild over already-loaded entries (dailySeries, group-by cycling, the budget view, applyReport, rates save, the edit form's "moved outside range" check) now reads through activeRange(); Home's label and the two load dispatchers keep currentRange(), since they describe the NEXT load, not the data already on screen.
…mutations (#28) TestRebuildKeepsTheRangeTheEntriesWereLoadedFor resolved start/end and dispatched Update under the same frozen clock, so a handler that recomputed the range instead of transporting it produced the same result — the 23:59:59 -> 00:00:01 latency window the task is about was never exercised at message-dispatch time. Advance now between resolving the range and dispatching the message to close that gap. TestBrowserReloadRepinsTheRange left year/month at their real construction-time default (this_month reads them directly, not m.now()), so a recompute mutation happened to match the fixture's July range purely by coincidence of when this suite is run; pin them to January instead. It also never asserted on the rebuilt report, only on activeRange(), so pinning the range after applyReport() instead of before (violating the handler's own "Pin BEFORE rebuilding" comment) went unnoticed; assert on report.Start too. Also drops a comment reference into the gitignored SDD brief directory, invisible to anyone reading the repo, and tightens rangeLabel's doc comment.
) Extract scrollWindow from scrollPalette so both screens share one scrolling idiom, and window the Filters view over its visual rows (section headers included) so long list/tag sets no longer run off the bottom of a short terminal with no way to reach them. view(th, rows) now takes an explicit row budget and filtersRows/ filtersContentRows/filtersCursorRow keep updateFilters's scrolling and the rendered window in agreement. filtersContentRows measures the title block's real cost (3 lines, not 2 — th.Title's own MarginBottom renders a second line) rather than assuming it, since the naive rows-2 offset overshot the row budget by 2 on every case.
…not the brief's literal (#28) TestFiltersWindowFollowsTheCursor computed its expected window as the literal filtersRows(m.height)-2 instead of filtersContentRows(filtersRows(m.height)), the expression updateFilters actually scrolls to. That literal was the brief's original (wrong) budget, so the test couldn't catch a regression back to it: reinstating filtersRows(m.height)-2 in updateFilters left the cursor genuinely off-screen (visual row 31 outside window [24, 30)) while the rest of the suite stayed green.
ClickUp's documented example returns task_tags as objects
({name, tag_fg, tag_bg, creator}), but the API reference's generated schema
for the date-range endpoint types it as an array of strings. A bare string
used to fail the WHOLE TimeEntries decode -- "cannot unmarshal string into
Go struct field rawEntry.data.task_tags" -- losing every entry, not just
the tags. rawEntry.TaskTags/Tags now decode through a tolerant tagName type
that accepts either shape.
Filter computed the per-dimension counts once for Empty() and again for its own loop; it now counts once and derives emptiness from those counts. Empty() itself is untouched (exported, used outside this package). The TUI carried three copies of the same loc/pricing/build/newReport sequence (report.go's GroupBy handler, the entriesMsg handler, and saveRates); they now share Model.rebuildReport(groupBy), with applyReport as the default-grouping wrapper. The entriesMsg handler still pins loadedStart/loadedEnd before calling it, so activeRange() inside rebuildReport reads the freshly loaded range.
Pins the fix from #4 (fixture days as offsets wrapped modulo the range span) so the non-month presets (last_7d, last_30d, this_week) never regress back to a near-empty demo report. Regression guard on an already-closed box: passes on the first run.
…eport applyReport's doc comment named only the pricing-parse failure, left over from before this task made it delegate to rebuildReport (which also fails on an unparseable timezone via locOrErr). Point at rebuildReport's own comment instead of re-listing the cases, so they can't diverge again.
Four Minor findings from tranche D's final whole-branch review, all pre-existing comments that drifted from what the code actually does: - report_table.go: the 52/60-column thresholds for Amount truncation were measured under the fixed 8+8 Hours/Billed reservation this branch removed; the threshold now depends on three independently measured widths, so the comment describes the mechanism instead of naming numbers that would age badly again. - report_table.go: the Amount-width floor of 1 was justified by a false claim that floor 0 removes the column entirely. Headers() passes "Amount" whole and lipgloss/table sizes columns from the header too, so the column never disappears; what floor 0 actually does is drop every data cell to "" with no ellipsis, which is the real reason for the floor. - footer_test.go: TestClampWidthNeverExceedsTheTerminal's comment still blamed TestMain's termenv.Ascii pin for making the style change invisible, contradicting footer.go's own (already-corrected) explanation that the style simply carries no color. Point the test comment at footer.go instead of repeating a different, wrong reason. - rates_view.go: the Cur column's %-5s was justified by treating the currency as a guaranteed ISO code "by construction". It is user config (Billing.Currencies/DefaultCurrency/Currency) with no format check in Valid(), so a wide-rune currency would misalign the row under %-5s (which pads by rune count). Switch to cell(cur, 5), matching every other non-ASCII column in this file, and add a test with a wide-rune currency fixture to guard the regression. gofmt, go vet, staticcheck, go build and go test -race all clean; no golden files moved.
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.
🇬🇧 English
Summary
Makes the layout invariants this package already claims in its comments actually true, and clears the remaining polish debt of #28.
Every screen measures its layout in display columns, but three helpers cut and padded by rune count — and Go's
fmtpads%-Nsby rune count too, so both halves of everytruncate(s,N)+%-Nspair were wrong. They agree only for ASCII, and ClickUp list and task names are exactly where emoji and CJK live. This branch puts one primitive under all of it (ansi.Truncate, already a direct dependency), then fixes the three measured layout defects that reservation-instead-of-measurement had produced.Totalgrouping — the one the report opens with — the table showed two rows with the same numbers,Totaldirectly aboveTOTAL, differing only in case and color.100%over 19 of 20 blocks.2 × (width − 8).time.Now()on every rebuild, so regrouping after midnight relabeled the report with a range the loaded entries never covered. The Filters screen had no scrolling at all. A bare-stringtask_tagsfailed the wholeTimeEntriesdecode, losing every entry rather than just the tags.Linked issue
Closes #137
Closes #136
Closes #135
Closes #141
Closes #28
Addresses point 1 of #138 — point 2 (giving
th.Boxa border color) moves every golden that uses it and belongs with the theme work, as that issue itself says, so #138 stays open.Follow-ups filed from the final review: #143 (the compositor lets the background bleed past the box; the report header is never clamped to the terminal) and #144 (the budget screen measures its percentage and figure widths but never pads to them).
Type of change
Checklist
go test ./... -racepassesgofmt,go vet,staticcheckare cleanCo-Authored-Byin commit messagesNotes for the reviewer
go.modandgo.sumare untouched.report_total,report_total_multicurrency) and one (filters_scrolled);budget.goldenshrinks from 94 to 80 columns;report/report_narrowgain 5 columns in the label column;palette_narrowgains one column because the table under the overlay stops truncating Amount needlessly.reportAmountWidthclaws space back from Amount as a last resort, which hand arithmetic misses.🇮🇹 Italiano
Riepilogo
Rende vere le invarianti di layout che questo package già dichiara nei propri commenti, e smarca il debito di rifinitura rimasto nella #28.
Ogni schermata misura il proprio layout in colonne di display, ma tre helper tagliavano e riempivano contando le rune — e il
fmtdi Go riempie%-Nsin rune, quindi sbagliavano entrambe le metà di ogni coppiatruncate(s,N)+%-Ns. Coincidono solo in ASCII, e i nomi di lista e task di ClickUp sono esattamente il posto dove vivono emoji e CJK. Questo branch mette una primitiva sola sotto tutto (ansi.Truncate, già dipendenza diretta), e poi corregge i tre difetti di layout misurati che la riserva-invece-della-misura aveva prodotto.Total— quello con cui il report si apre — la tabella mostrava due righe con gli stessi numeri,Totalsubito sopraTOTAL, diverse solo per maiuscole e colore.100%sopra 19 blocchi su 20.2 × (larghezza − 8).time.Now()a ogni ricostruzione, quindi raggruppare dopo mezzanotte rietichettava il report con un periodo che i dati caricati non coprivano. La schermata Filters non aveva alcuno scorrimento. Untask_tagsa stringhe nude faceva fallire l'intero decode diTimeEntries, perdendo tutte le entry e non solo i tag.Issue collegata
Closes #137
Closes #136
Closes #135
Closes #141
Closes #28
Chiude il primo punto della #138 — il secondo (dare un colore bordo a
th.Box) muove i golden di ogni schermata che lo usa e appartiene al lavoro sui temi, come dice la issue stessa, quindi la #138 resta aperta.Follow-up aperti dalla review finale: #143 (il compositore lascia sporgere il fondo accanto al box; l'header del report non è mai troncato alla larghezza del terminale) e #144 (la schermata budget misura le larghezze di percentuale e cifre e poi non ci allinea niente).
Tipo di modifica
Checklist
go test ./... -racepassagofmt,go vet,staticcheckpulitiCo-Authored-Bynei messaggi di commitNote per chi rivede
go.modego.sumsono intatti.report_total,report_total_multicurrency) più uno (filters_scrolled);budget.goldenscende da 94 a 80 colonne;reportereport_narrowguadagnano 5 colonne nella colonna delle etichette;palette_narrowguadagna una colonna perché la tabella sotto l'overlay smette di troncare Amount inutilmente.reportAmountWidthriprende spazio ad Amount come ultima risorsa, e l'aritmetica a mano non lo vede.