Stabilize TUI refresh, scrolling, responsive layout, and dashboard data density - #863
Stabilize TUI refresh, scrolling, responsive layout, and dashboard data density#863ihearttokyo wants to merge 5 commits into
Conversation
Reconciled submission —
|
ihearttokyo
changed the title
Final validation update —
|
ihearttokyo
changed the title
ozymandiashh
left a comment
ozymandiashh
left a comment
There was a problem hiding this comment.
Thanks for this. The scrolling, refresh-race and adaptive-layout work is careful, and I traced a fair bit of it by hand: the generation-counter and pending-ref coordination in reloadData, the viewRef stale-closure avoidance, the shared getDailyActivityPageSize / getProjectBreakdownRowLimit calculation that fixes the drift you describe, and the shortProject truncation staging. All of it holds up, and the test coverage for the stated behaviour is genuinely strong.
One thing should block, though, because it is invisible to CI and to this PR's own tests by construction.
The resize handler reintroduces the Windows/ConPTY hang from #195
src/dashboard.tsx:1504
process.stdout.write('\u001B[?2026h\u001B[2J\u001B[H')That writes Begin-Synchronized-Update directly to stdout. The repo already has a guard for exactly this sequence, added by c511627 to close #195, whose commit message states that ConPTY "does not implement this protocol and buffers indefinitely, causing the dashboard to hang with no output":
// src/ink-win.ts:11
if (chunk === BSU || chunk === ESU) return trueThe filter is an exact string comparison. The write above is BSU concatenated with clear-screen and cursor-home, so it is a different string, chunk === BSU is false, and the raw BSU passes straight through to the terminal on Windows. Every resize would then trigger the failure mode that guard exists to prevent.
There is also no matching End-Synchronized-Update anywhere in the diff. I grepped the branch: \u001B[?2026l appears zero times. The region is left open and only closes because Ink's next internal frame happens to emit its own correctly paired bsu/esu.
Neither the Windows package-build check nor tests/dashboard.test.ts can catch this. The tests drive PassThrough streams and assert on stripAnsied substrings, so they cannot observe ConPTY buffering or cursor desync.
Ink 7 already listens for resize on stdout and performs a layout recalc plus a full unthrottled re-render (ink.js:261-272), wrapped in its own correct bsu/esu pairing, so the manual write may simply be unnecessary. If a hard clear really is needed, gating it behind process.platform !== 'win32' would at least match the reasoning already encoded in ink-win.ts.
Smaller things, none blocking
-
The
isHeavyPeriodauto-refresh gate is gone. The previous code didif (!dayDate && isHeavyPeriod(period)) return, skipping auto-refresh on 30days/month/all/lifetime.src/dashboard.tsx:1270-1276no longer has it, so background reparses now fire on heavy tabs.getDashboardScanRangebounds the scan whenscrollableHistoryis set, so this may well be fine, but it is a behaviour change the description does not mention and it would be good to hear it was deliberate. -
Aggregation runs inside render.
dailyHistoryPageSize,dailyHistoryRowCountandmodelCountare computed in theInteractiveDashboardbody (src/dashboard.tsx:1132-1144) rather than inuseMemo, so they recompute on every paging keystroke. SeparatelyaggregateModelTotals(projects)runs twice per data-changing render, once inside the memoizedgetDashboardMaxWidthand again unmemoized inModelBreakdown. -
ScrollableViewport'suseLayoutEffecthas no dependency array (src/dashboard.tsx:1062-1067), someasureElementruns after every render including every scroll tick, even though content height does not change while scrolling. -
StatusBar scrolls away. It is the last child of
content, which is entirely insideScrollableViewport(:1431-1439), so the keybinding hints disappear when you scroll down on a tall dashboard. Pinning it outside the scroll region would keep them visible. -
--refreshhelp text does not mention the new 60s floor.src/main.ts:767,1197,1215still say "Auto-refresh interval in seconds (0 to disable)", so--refresh 10silently becomes 60 with only the README explaining why. -
src/compare.tsxstill caps at 2 columns (:214-215,257), so pressingcon a 135+ column terminal visibly narrows from the new 3-column layout. Pre-existing duplication rather than something this PR introduced, but the gap is wider now.
Everything else checked out on my side: tsc --noEmit is clean, and tests/dashboard.test.ts passes in full. The other failures I saw in full-suite runs (cli-emitters, cli-status-menubar, cache-refresh-lock) reproduce on unmodified main in my environment and are not attributable to this branch.
Happy to re-review once the resize write is sorted.
|
The branch allows maintainer edits, so I pushed 7716f95 with the targeted fix for the one blocking finding in my review, rather than leaving it to stall. @ihearttokyo it stays your PR, amend freely. The commit keeps the resize reset but emits the synchronized-update escapes as standalone writes, sourced from Behavior by platform: Windows gets clear+home only (BSU/ESU filtered), everything else gets All 48 dashboard tests pass locally on the branch, Since the fix is my own code I'm not converting my review into an approval; the remaining call is a maintainer's. Everything else in the PR I had already traced and found solid (generation-counter reload coordination, viewport preservation, the shared row-limit calculation, truncation staging), so from my side this is now unblocked. |
|
Status for whoever picks this up (@iamtoruk): the branch now carries the ConPTY fix (7716f95) and I verified locally what CI would check and more: Two things need a call I should not make alone:
From my side the PR is technically unblocked. |
Keep background refreshes from blanking or replacing the active Optimize view, and enforce a one-minute minimum refresh interval.\n\nRework the dashboard into a stable 3/2/1-column flow with left-aligned bars, justified metric columns, readable project headings, and a ten-row Daily Activity viewport.\n\nSynchronize resize state before Ink paints so breakpoint transitions do not leave stale frames, preserve content beyond 256 terminal columns, and cap the dashboard at the current data's renderable width. Add focused regression coverage and a submission statement documenting live Ghostty validation.
Pin the interactive dashboard to a terminal-sized viewport and add line, page, home, and end navigation without sacrificing the alternate-screen resize protections. Preserve the viewport offset across background refreshes and ordinary rerenders while resetting cleanly for a new view or period. Give daily-history paging its own Space binding, render full model costs whenever the panel can hold them, and spell out the project session heading. Extend the responsive dashboard regressions across one-, two-, and three-column viewports and update the submission evidence.
Keep every dashboard metric visible with intrinsic column widths and one cell of separation, allowing bars and project labels to yield space before headings or values disappear. Shorten project paths in meaningful stages so the project title remains recognizable for as long as possible. Derive Daily Activity's page size from the sibling panels in the active responsive row: ten dates in one column, the visible project count in two, and the greater project or activity count in three. Reuse that calculation for rendering, cursor bounds, paging, and status text so the viewport cannot drift from its navigation contract. Cover the behavior with live Ink regressions, path-shortening contracts, the 70-test dashboard/model/overview matrix, and the rebuilt submission record.
The resize reset wrote Begin-Synchronized-Update concatenated with clear+home in a single chunk. ink-win's ConPTY filter compares chunks exactly, so the raw BSU passed through on Windows and would reintroduce the getagentseal#195 hang; the update was also never ended, leaving terminals that do implement 2026 to rely on their timeout. BSU/ESU now live in ink-win.ts and are written standalone: swallowed by the Windows filter, honored elsewhere, and properly closed around the clear.
Remove application-owned synchronized terminal writes so Ink remains the single resize synchronization owner and the Windows ConPTY filter retains its upstream contract. Restore the aggregate-period refresh gate, document the one-minute floor in all CLI help surfaces, and lock both policies with focused regressions. Rebuild the submission record around the rebased branch, extensive shrink-heavy Ghostty evidence, upstream-baseline failures, and the maintainer review.
4df066d to
47968c1
Compare
|
@ozymandiashh @iamtoruk — thank you for tracing the ConPTY path and for the targeted maintainer fix. I reconciled that review by removing CodeBurn's custom synchronized-update writes entirely: I also restored the heavy-period refresh gate, corrected the CLI help, reran the full focused and broader matrices, and completed a shrink-heavy native Ghostty sweep. The refreshed head is |
2 participants
Summary
Maintainer review reconciliation
The prior custom synchronized-update write has been removed.
src/ink-win.tsis restored to upstream, Ink exclusively owns terminal synchronization, and CodeBurn's prepended resize handler only captures the new width and rerenders React. This eliminates the reviewed ConPTY escape path rather than maintaining another platform-specific protocol.The reconciliation also restores the existing policy that Today, 7 Days, and concrete-day views may auto-refresh while 30 Days, Month, All, and Lifetime remain static. All three CLI help surfaces now state the 60-second minimum and the
0disable value.Testing
npm testpasses — the supported root-only run has 2,481 passing, 3 failing, and 5 skipped tests; all three failures reproduce unchanged on upstream2c3319b. The separately configured desktop suite passes 462/462.npm run buildsucceedsAdditional gates:
Full methodology and evidence are recorded in
SUBMISSION.md.Reviewer focus
Please focus on the interaction among shared metric sizing, adaptive Daily Activity page size, and scroll state. Background refresh must preserve view and position. Supported widths must retain every metric, settled resize must preserve panel order and content, and navigation must use the page size displayed on screen.