mirror of
https://github.com/l0ng-ai/tty7.git
synced 2026-09-21 16:02:20 +00:00
f6fa34a016
* feat(diff): make the sidebar diff preview optional and bound its cost Clicking a sidebar row's `+N −N` opens the working-tree diff overlay. On a big tree that could stall the window, and not everyone wants an in-app diff viewer in the first place. Two halves, matching the report. The setting: `sidebar_diff_preview` (Settings → Window & Tabs, on by default, persisted in `config.json`). Off, the branch and the counts stay exactly where they are and read exactly the same; they lose only the pointer cursor and the `toggle_diff_overlay` handler, so the press falls through to ordinary tab activation. Both come off one value — `diff_click_cwd` — so they cannot get out of step. The performance work. All five of the reporter's hypotheses held up against v26.7.6, and each fix is measured on a 300-file / 90 000-line / 4.5 MB diff (release, macOS arm64): 1. The full diff was buffered before parsing — `git_status::git` uses `Command::output()`. Now streamed line by line through the new `git_status::git_lines` into an incremental `DiffParser`: peak transient buffer 4 552 060 bytes → 50 bytes, at ~1.7× the parse CPU (3.97 ms → 6.76 ms) on the background thread, where it never touches a frame. 2. The snapshot was deep-cloned per holder inside `this.update`, i.e. on the UI thread. Now shared behind `Arc`: 2.41 ms → 11 ns per holder. 3. The element tree is not virtualized — confirmed, not cured. Rendering is not being redesigned here; instead the element count is bounded (see 4) and `MAX_RENDERED_FILES` caps the cards built at all, with a "… and N more" line for the tail. 4. Auto-collapse was per file, and counted only +/− while the rendered body also has context lines. Added `AUTO_COLLAPSE_TOTAL_LINES` over *retained* lines: sixty forty-line files, none individually large, went from 2400 side-by-side rows to zero, under a summary saying the diff is too large to render efficiently and pointing at expanding individual files or `git diff`. 5. The Changes panel probed independently and kept its own snapshot. Both now go through `spawn_shared_diff_probe`, which dedupes by cwd and installs one `Arc` into every watcher; opening the overlay while the panel already shows that repo now paints from the panel's snapshot instead of re-probing. Plus a repo-wide retention budget (`MAX_TOTAL_LINES`, `MAX_FILES_WITH_HUNKS`): 90 000 lines / 6.2 MiB of line text → 20 000 / 1.2 MiB. The `+N −N` totals deliberately escape every cap — they are compared against `--numstat` to detect staleness, so a capped total would disagree forever and re-probe in a loop. Small diffs are untouched: a forty-file, twelve-lines-each tree is not oversized and still opens expanded, asserted directly. Not verified: anything requiring the GUI. No frame timings, no visual check of the oversized banner or the settings row, and `AUTO_COLLAPSE_TOTAL_LINES` is a judgement call anchored on row count rather than a measured frame budget. Refs #239. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(diff): bound the untracked list and stop a moving default flipping toggles Five findings from the review of the previous commit, all confirmed against the source before acting. The untracked list escaped every bound the previous commit added. `git ls-files --others` reports the whole tree of anything not yet ignored, so a fresh clone before `node_modules` / `target` / `.venv` reach `.gitignore` answers with tens of thousands of paths — read through the buffering helper, retained without a cap, ignored by `oversized()`, and rendered one non-virtualized row each. That reaches the overlay without going through the diff at all, which is why the diff budget never saw it. It is now streamed through `git_lines`, capped at `MAX_UNTRACKED`, counted toward the oversized threshold, and rendered at most `MAX_RENDERED_FILES` rows with a "… and N more" tail. The reported count stays the true total via `untracked_total` — same split the diff side already makes between what is retained and what is counted, because a count that shrank with the budget would read as files having disappeared. The overlay's expand state was an inversion set — "files flipped away from their default" — which was correct while the default was per-file and stable. The repo-wide `collapse_all` moves the default for every file at once, so any refresh crossing the oversized threshold inverted every explicit choice simultaneously: the two files the user had opened snapped shut and the rest sprang open. It now stores absolute intent (`HashMap<String, bool>`), answered before the default is even computed, so nothing about the snapshot can reach it. Covered by a test that asserts an explicitly opened and an explicitly closed file both survive a transition in either direction while an untouched file follows the default. The Changes panel dropped freshly landed snapshots. `install_diff_snapshot` only wrote `right_panel.diff` when the panel was the one waiting, so a probe the overlay started was discarded even when the panel sat on that exact repo — the overlay rendered the new snapshot while the panel kept the old one, same window, same repo. The wait (`diff_pending`) and the data (`diff_cwd`) are now claimed separately: with probes deduped per cwd there is at most one in flight, so there is no out-of-order overwrite to guard against. The oversized banner reported `retained_lines()` as "diff lines", which after the budget fires is what was kept, not what changed — it read "20000 diff lines" directly under a header showing the exact +90000/-0. It now states loaded-of-total and names which cap ate the difference, and lists each axis that tripped the threshold so a big untracked list never reads as a claim that the diff is big. And the Changes panel deep-copied every untracked path String on every frame, on the UI thread, for two `len()`/`is_empty()` reads — the same cost class the `Arc` switch removed from the probe path. 995 tests pass, fmt clean, no new clippy warnings. The app was not built, launched, or driven; visual acceptance is the owner's. Refs #239. * no-mistakes(review): cap Changes panel rows and fix oversized banner truncation notice * no-mistakes(review): drive banner per-file truncation off parser flags * no-mistakes(document): correct changelog cost count and overlay-trigger docs * fix(diff): reconcile the diff-overlay work with the host-aware git refactor The rebase onto main lands this change on top of the remote-workspace work (#235, #242), which moved the git helpers into `tty7-core` and made every read take the pane's `Host`. Adapting rather than papering over: - `sidebar_diff_preview` moves to the core `Config`, where the struct now lives. - The diff and untracked reads go through `git_status::git(host, cwd, ..)`. - The shared probe, its in-flight set, and `install_diff_snapshot` key on (`HostId`, `PathBuf`) — the same path on two machines is two work trees — as does the Changes panel's `diff_pending`. - `diff_click_cwd` became generic over what identifies a repo, so the setting gate did not need to learn about hosts. - Main's newer card rounding reads `truncated`, which is an `Option` here now. The streaming diff read is deliberately absent at this commit: `Host::git` is buffered, so it is restored on top of a streaming host API in the next one. Refs #239. * feat(host): stream git reads whose size scales with the work tree `Host::git` returns a fully-buffered `Output`, and the remote implementation round-trips one over the wire. That is the right shape for the reads tty7 does constantly — `rev-parse`, `symbolic-ref`, `--numstat` — all of which answer in bytes. It is the wrong shape for `git diff HEAD`, whose output scales with the work tree rather than with anything the UI can show: this repository's own `git log -p -n 400` is 8.3 MB, and the diff overlay keeps a small fraction of it. So `Host` grows a second entry point rather than changing the first. `git_lines` delivers the same invocation a line at a time, and its **default implementation buffers** — every host gets it for free, nothing that works today changes shape, and it stays inside the "every git read funnels through the host" invariant instead of becoming a way around it. Overriding it is an optimisation, never a behaviour change: a test asserts the streamed and buffered reads yield identical lines. The local host reads straight off the pipe. The remote host adds `ControlRequest::GitStream`, answered with `ControlEvent::GitChunk` pushes and a terminating `GitEnd` carrying the exit status — the shape `WatchOpen` already proved out. Two protocol details worth the reader's attention: The **client** picks the stream id, which is why the reply carries none. Ids only need to be unique within a connection and a connection has one client, so choosing it client-side lets the receiver be registered *before* the request goes out. A server-assigned id arrives in the reply, leaving a window where a chunk that overtook it reaches a client with no entry for that id and is dropped under the unknown-id rule — silently losing the front of the diff. `WatchOpen` needs a whole deferred-start mechanism to close that window; this does not. The feature is **advertised and checked**, not assumed. A server predating `GitStream` cannot decode the variant, and an undecodable frame ends the connection — so sending it blind would not degrade, it would disconnect. Servers advertise `git-stream`; a client that does not see it uses the buffered `Git`, which is the path every remote pane used before this existed. Covered by a test against a peer advertising only `control` and `host-rpc`. Chunks carry newline-terminated line data batched to ~64 KiB, not verbatim slices of stdout: the server reads through `git_lines` itself, so line content survives exactly while `\r\n` and a missing final terminator are normalised away. The only consumer is line-oriented. Framing per batch rather than per line is what keeps a 90 000-line diff from becoming 90 000 frames. Re-measured on the rebased code, against real git output, release build (the previous figure was taken before the host refactor and no longer holds): buffered: 8 269 409 bytes resident, read 817 ms + parse 12 ms streamed: peak transient chunk 64 KiB, read+parse 557 ms Lower peak memory *and* faster end to end — parsing now overlaps with git producing output instead of waiting for all of it. The earlier synthetic measurement showed streaming costing ~1.7x CPU; that was an artifact of reading a warm page-cached file, where there was nothing to overlap with. The app was not built, launched, or driven; visual acceptance is the owner's. Refs #239. * fix(host): make unsubscribing a watch take effect at the drop, not after it CI's Windows job failed `watch_drop_unsubscribes`: an event for a file created *after* the subscription was dropped still reached a consumer holding a clone of the receiver. Tearing the watcher down is not instantaneous. The OS backend runs its own thread, and on Windows a `ReadDirectoryChangesW` completion can fire during teardown, reach the event closure while `raw_tx` is still alive, and be forwarded by a coalescer that has not yet noticed the disconnect. So "dropped" meant "stops delivering shortly", which is not what the subscription promises — and for a remote host it is the difference between releasing a server-side watch and leaking one. The handle now closes the delivery channel in its own `Drop`, before any of that unwinds. Batches already queued stay readable — `close` stops sends, not receives — which is the one thing a consumer racing its own drop may legitimately still see, and exactly what the conformance test allows for. Not this branch's bug: the change here is to git reads, not watches. But main is flaky in the same family — it failed the sibling `watch_coalesces_within_window` eleven hours ago and was hardened for that one — so this fixes the cause rather than loosening the test. Refs #239. * refactor(host): make the streaming git read part of the protocol Remote workspaces have never shipped a release, so there is no deployed server to negotiate with. The `git-stream` feature flag, the `has_feature` check and the buffered fallback behind it were all guarding against a peer that cannot exist — dead code that would have to be maintained, and read by the next person as evidence that older servers are out there. `ControlRequest::GitStream` is simply part of the control protocol now. Buffered `Host::git` stays exactly as it was, for the many reads that answer in bytes and have no reason to stream. The remote test that proved the fallback becomes one that proves the stream: the peer serves `GitStream`, splits a line across two chunks, and the client reassembles it — the case the reassembly exists for. Refs #239. * no-mistakes(review): fix remote git-stream deadlock, chunk encoding and stray docs * no-mistakes(review): stop git-stream batch growing after a send failure * no-mistakes(document): note git-stream protocol delta in remote-workspace contract doc * fix(host): bound a git stream's wait, its lines, and its concurrency Three ways the streaming git read could still hold or hang more than it should, all found reviewing #239's implementation. A stream is answered by pushes, so neither of the failure paths the rest of the client relies on covers it: the request deadline was satisfied by the immediate `Unit` reply, and keepalive watches the link, which stays up while a server-side git wedges on a network filesystem. The reader parked forever, on one of a small pool of blocking threads, and the diff probe it belonged to never released its per-repo claim — so that repository's overlay and Changes panel were stuck on "Loading…" for the life of the process. `git_lines` now waits `GIT_STREAM_IDLE_TIMEOUT` between chunks. Between, not across: a slow-but-alive read must be allowed to take as long as it takes, which is why a total deadline would be the wrong instrument. Draining moved to `drain_git_stream` so all three exits are reachable from a test without waiting out two minutes. "Incremental" bounded the number of allocations but not the size of any one of them: a line is only complete at its newline, so a work tree with a minified bundle rebuilt the whole-output peak inside `LineSplitter`, on both ends of a remote link and in the server's outgoing batch. Lines are now capped at `MAX_LINE`, and what is cut says so in the line itself rather than silently shortening a rendered diff. That also bounds the server batch, which makes `GIT_STREAM_CHUNK_MAX` a frame backstop rather than the only thing standing between a bundle and a 32 MiB payload. Finally, `GitStream` is the one request that spawns a thread outside the bounded worker pool, so nothing counted them. `MAX_CONCURRENT_GIT_STREAMS` per connection now does, with the slot returned by a guard so a refusal, a failed spawn and a panicking read all give it back — a leaked slot would be a permanent refusal, not a transient one. * fix(diff): stop the overlay re-walking the tree per frame, and re-probe a folded-in refresh Two things the shared-probe work left on the render path. The overlay asks six whole-snapshot questions while building its element tree — oversized, totals, retained lines, budget fired, per-file cap fired, untracked count — and each accessor walked `files` on its own, `oversized` walking the hunks too. `files` is deliberately uncapped (only hunks are), so that was six walks over a list whose length is the size of the working tree, on the UI thread, on exactly the tree this module exists to keep responsive. `DiffSnapshot::stats` answers all six in one pass and the per-question accessors are gone, so nothing can drift from it. Computed rather than stored, because the snapshot is built by hand with `..Default::default()` throughout the tests and a cached count would read as zero for every one of them. Deduping probes per repository is what makes one `git diff` answer every watcher, but a probe describes the tree as it was when it *started*. A refresh triggered after that — a command finished, an agent turn ended — folded into the running probe and was answered with a snapshot already known to be stale, with nothing left to trigger another look: the overlay's own re-check is gated on `loading`, which the landing clears, and the `GitStatusCache` change that would have re-armed it has been spent. A folded-in request is now remembered and re-issued when that probe lands. It converges rather than loops, because a quiet tree never sets the flag. * fix(diff): bound the stream queue, and stop two thresholds answering the wrong question Review follow-ups on the sidebar-diff branch. Five findings, four of them about a bound that was claimed but not held. The remote streaming read bounded both ends and not the middle. The reader thread serves the whole connection, so it cannot wait on a slow consumer — parking it there stalls every other reply and the keepalive with it — and an unbounded queue was the price. That reassembles the whole diff in a channel, which is the peak the buffered read was replaced to avoid, one container further along. The queue is now bounded instead of back-pressured: each chunk is charged to the stream's arrears, the drainer credits them back, and a stream 32 MiB behind is cut loose with an error rather than served. Real back-pressure would need credit-based flow control in the dialect; this is not that, and says so. `oversized` counted untracked paths on its file axis, and collapsing every file body removes no untracked rows — that section has no bodies to fold. A tree with an un-ignored node_modules and three edited files hid the three cheap things, kept the expensive one, and told the reader their working tree was too large to render. The untracked list is bounded where it is built: MAX_UNTRACKED on retention, MAX_RENDERED_FILES on rows. AUTO_COLLAPSE_TOTAL_LINES counted the context lines git prints around every hunk — four to six retained per line actually changed — against a threshold set as if it were reading `+N -N`. It fired on trees whose header said 400. 8000, compared against the 20000 the parser stops retaining at, since collapsing everything is the heavier of the two interventions and should not arrive first by much. A probe that could not run produced an empty file list, which renders as "Working tree clean" — a claim about the repository, made because a read timed out. Newly reachable, too: a stream can be refused or go silent where a buffered read could only arrive or error. DiffSnapshot::read_failed keeps the two apart. Also: StreamStop's doc comment had been glued onto StreamSlot, leaving the enum undocumented and the guard described twice; the overlay header asked totals() beside stats() rather than through it; and the watch-teardown fix riding along on this branch was in neither the PR body nor the CHANGELOG. Tests: the queue budget both ways (a stream that outruns it is cut loose, a larger one that is drained is not), the untracked axis, an ordinary context-heavy afternoon sized to fail against the old threshold and pass against the new, and empty-because-broken against empty-because-clean. Each was checked to fail against the behaviour it replaces. 1603 pass, 0 fail; fmt clean; no new clippy warnings in the touched files. --------- Co-authored-by: l0ng-ai <24760907+l0ng-ai@users.noreply.github.com> Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>