Skip to content

perf(tui): refresh architecture v2 — adaptive tick + generation skip (#1753) - #1765

Open
asheshgoplani wants to merge 2 commits into
mainfrom
perf/refresh-tick-v2
Open

perf(tui): refresh architecture v2 — adaptive tick + generation skip (#1753)#1765
asheshgoplani wants to merge 2 commits into
mainfrom
perf/refresh-tick-v2

Conversation

@asheshgoplani

@asheshgoplani asheshgoplani commented Jul 26, 2026

Copy link
Copy Markdown
Owner

The problem

The refresh tick does work proportional to all live sessions. #1756 fixed the
render half of #1753 (full-frame MaxWidth rebuild → measure-then-rebuild, CPU
4.84% → 2.04%). What remains is the status sweep, and it is the structurally
wrong shape: it re-derives state for sessions that provably did not change, using
the most expensive mechanism available.

Three facts compose badly:

  1. Almost nothing has a control pipe. livePipeLRUCapacity = 3 + attached
    sessions — deliberate, since pipes cost a tmux client each. So the sweep's
    existing "PipeManager saw no output for 5s → skip" fast path covers ~4 rows
    out of 70. Everyone else falls through.
  2. Without a pipe, CapturePane is a subprocess. tmux capture-pane -p -e,
    3s timeout, serialized by the tmux server, cached 500ms.
  3. The most common steady state on a big deck captures the pane. A Claude
    session parked in hook waiting (done, needs you) takes the hook fast path,
    which calls BackgroundWorkPending() to check for in-flight
    run_in_background work — a capture, cached bgWorkCacheTTL = 3s. With a 2s
    sweep that is a capture roughly every other sweep, per waiting session.

At ~60 quiescent sessions that is tens of capture-pane subprocesses per sweep,
queued behind one tmux server, to re-derive statuses identical to the ones
already on screen. And when the sweep overruns, nextStatusInterval backs off —
so the system degrades by getting less fresh, not by getting cheaper.

And the live-fleet diagnostic reframed the requirement: the lag reproduces
when a group containing many sessions is expanded, and collapsing it makes
it fine. With a large group expanded, visible-row count approaches fleet size —
so any design that treats visible rows as must-refresh-every-tick degenerates
back to the full per-row storm exactly when the user is looking at the most
rows. Visible rows need a cheap-evidence skip and a per-tick budget too.

The design (docs/design/refresh-architecture-v2.md)

Event-driven first. Claude/Codex/Gemini/Hermes/Cursor already push
lifecycle events into hook files, and session.StatusFileWatcher already
watches that directory with fsnotify and maintains a live status map. But it is
constructed as NewStatusFileWatcher(nil)onChange is nil. Nothing
wakes the TUI; the sweep polls the watcher's map on its own cadence. We pay for
a push pipeline and consume it by polling. Target: hooks and tmux control-mode
notifications cause updates; the periodic sweep demotes to a reconciliation
safety net for what events cannot report and can then run at 10–30s.

Adaptive second. Rows refresh when there is evidence they changed;
generation counters make unchanged sessions cost zero; work per tick is
budgeted rather than unbounded — for visible rows too, because the group-expand
case makes "visible" as large as the fleet.

Remote-ready third. The roadmap is fleets on cloud machines with the Mac as
a thin control surface. "Subprocess per session per tick" is not merely slower
over SSH, it is a different order of magnitude — and both principles above
carry over: an event crossing an SSH connection costs one multiplexed message,
and a generation counter lets a remote answer "nothing changed" once for the
whole host. So the refresh layer should treat a host as the unit of cost:
Mac cost O(hosts), independent of session count. Existing --ssh support is
the seed; v2 generalizes that and makes local tmux one implementation of a
session host behind it.

The doc also records the invariants any future change to this layer must
preserve (never skip on absent information; never skip without a bound; fail
open; never infer death from silence; always ship a kill switch).

The slice in this PR

Per-session fingerprint (internal/ui/refresh_policy.go), built entirely
from caches the sweep already refreshed once per tick — window_activity from
the list-sessions snapshot, pane title/command/dead from the list-panes
snapshot, hook UpdatedAt, SSE UpdatedAt. Building it for the whole deck is a
handful of map lookups and spawns nothing. Incomplete evidence sets ok = false,
which always forces a poll.

The off-screen gate, right after the existing PipeManager idle-skip.
UpdateStatus is skipped only when every one of these holds:

  1. the row is not visible (visible rows go through the hold + budget below);
  2. the status is running / waiting / idle (error/stopped keep their own
    recheck timers, starting must converge fast);
  3. the fingerprint is usable and identical to the last polled one;
  4. fewer than maxSkips consecutive skips have happened.

Any veto polls. No path skips on absent information.

Visible rows: hold + budget (the group-expand case):

  • a visible row with a settled status and an unchanged fingerprint is held
    under the same maxSkips ceiling (refreshLedger.holdVisible) — an expanded
    quiescent group costs ~zero;
  • visible rows that do need a poll compete for a per-sweep budget
    (visiblePollBudgetPerSweep = 10, the sweep's worker-pool width); deferred
    rows age a starvation counter and are admitted first on later sweeps, so the
    budget cycles round-robin and every due row is polled within
    ceil(due/budget) sweeps;
  • the cursor row is always admitted — it drives the preview pane;
  • the incremental visible-first pass in processStatusUpdate, which used to
    poll every visible row per pass, now holds steady rows read-only
    (heldSteady; no ceiling, because the background sweep owns freshness) and
    budgets + round-robins the rest;
  • group expand is a trigger, not a burst: the keypress only rebuilds the
    flat list (rows render instantly from the existing render snapshot) and
    republishes the viewport immediately, so the next sweep amortizes the
    catch-up through the budget. No path between the keypress and the first
    frame spawns per-row tmux work.

Why this is safe

  • Not a new assumption. Session.GetStatus already gates its CapturePane
    on exactly "did window_activity change" (needsBusyCheck), and
    UpdateStatus gates its idle tier on it. This gate reuses that invariant and
    is strictly more conservative, because those skip for as long as activity
    is unchanged while this one is bounded by the ceiling.
  • The ceiling bounds the only real exposure. UpdateStatus has time-based
    transitions with no external evidence (acknowledged → idle, hook-freshness
    expiry, debounceFlipFromRunning confirmation, Hermes gateway recheck).
    The ceiling delays those by at most maxSkips sweeps (~6s at default)
    instead of suppressing them — where the existing needsBusyCheck has no
    bound at all. The same bound now covers visible rows; anything the user can
    see is at most ~3 sweeps stale, and only if nothing observable moved.
  • Real transitions are not delayed. Anything that happens moves the
    fingerprint on the next sweep: Stop hook → hookAt, any output →
    activity, spinner start/stop → title (Claude's OSC marker), exit under
    remain-on-exit → dead, session gone from tmux → ok=false → forced poll,
    OpenCode SSE → sseAt. A row with a moved fingerprint is polled — for
    visible rows within the budget, cursor first, starved rows first.
  • Fails open. With no viewport snapshot, or one older than 10s (event loop
    suspended by tea.Exec, or wedged), gate and budget are bypassed and
    every session is polled, exactly as before. A nil ledger also polls.
  • Attach-return can't be stale. refreshAttachedSessionStatus already
    clears hook status and forces a check; it also drops the fingerprint
    baseline — and that wiring is now pinned by a test that fails if the line is
    deleted.

Render-snapshot generation skip. refreshSessionRenderSnapshot is called
from six sweep/tick paths and used to allocate and publish a fresh N-entry map
every time. It now compares first and returns without allocating or storing when
nothing changed.

Kill switch[ui] adaptive_refresh_max_skips, resolved through
UISettings.GetAdaptiveRefreshMaxSkips (same clamp pattern as
GetRemoteSessionRefreshSecs):

value behaviour
0 / unset default 2 — poll every quiescent session at least every 3rd sweep (~6s)
>0 custom ceiling, clamped to 30 (MaxAdaptiveRefreshMaxSkips); it previously accepted 100000 verbatim, a ~55h staleness ceiling
<0 disable — every sweep polls every session, byte-identical to pre-policy behaviour; hold and budget both off

The knob resolves once, at TUI construction — changing it (including the
kill switch) takes effect on the next TUI restart, not on config reload.

Measured effect

Sandboxed #1753 harness, run on this branch: throwaway HOME, blank XDG, own
profile, own tmux socket, 60 synthetic --cmd "sleep 3600" sessions, terminal
220×50 (~44 of 60 rows visible — i.e. the group-expanded shape), 60s warmup +
240 single-key j/k events + 60s settle. Same binary both passes; before =
kill switch (adaptive_refresh_max_skips = -1, byte-identical to pre-policy),
after = default. Counters from the idle_sessions_skipped debug line this PR
added:

before (kill switch) after (default)
UpdateStatus calls per sweep (median) 60 of 60 10 of 60
UpdateStatus calls per sweep (mean) 60 16.5
adaptive_skipped per sweep (mean) 0 32.2
visible_deferred per sweep (mean) 0 11.3
slow_status_loop (>500ms) events 1 (639ms @ 60 sessions) 0
status transitions observed 120 120 (identical; 0 flicker)

The median sweep now polls 10 sessions instead of 60 — an ~83% reduction in
UpdateStatus calls
(mean ~72%) — while both passes converged through the
identical 120 status transitions, i.e. the policy changed cost, not outcomes.

Tests

internal/ui/refresh_policy_test.go — pure, no tmux server and no HOME:

  • every veto in the off-screen decision function, table-driven;
  • the staleness-ceiling pattern (skip, skip, poll) and counter reset on
    fingerprint change; forget()/prune()/nil-receiver behaviour;
  • visible hold: ceiling respected, all vetoes (changed/unusable
    fingerprint, unsettled status, kill switch, no baseline, nil ledger), and
    refused holds not consuming the ceiling;
  • read-only hold (heldSteady): 50 held passes consume none of the sweep
    ceiling;
  • budget round-robin: 25 due rows at budget 10 are all polled within 3
    sweeps (starvation-counter aging), and the cursor row is never deferred;
  • fingerprint evidence vs no-evidence, pane-title sensitivity, and — new —
    the hook/SSE inputs: a delivered hook or SSE sample moves hookAt/sseAt
    and breaks the match even with unchanged tmux evidence (this is what makes
    "real transitions are not delayed" true), via new watcher test seams;
  • viewport publication (now including cursorID) and both fail-open paths;
  • the render-snapshot skip asserted by map identity.

internal/ui/refresh_wiring_test.go — drives the real
backgroundStatusUpdate / refreshAttachedSessionStatus on a constructed
Home, so the wiring lines themselves are pinned (deleting either safety line
turns the suite red — see the review-response comment for the exact mapping).

internal/session/userconfig_adaptive_refresh_test.go — the clamp:
default / passthrough / max / above-max / 100000 / kill-switch cases.

tmux.SeedServerAliveForTest was added alongside the existing seed helpers so
internal/ui can drive the sweep deterministically with or without a tmux
server.

Follow-ups (roadmap in the doc, each independently shippable)

  1. Wire onChange on StatusFileWatcher → single-session refresh on hook
    delivery. First genuinely event-driven step, and it feeds the TUI-truth
    waiting = done vs needs-you split.
  2. Relax the safety-net sweep cadence once (1) lands.
  3. Stagger the attach-return catch-up burst.
  4. Fleet notification channel — one control-mode client per tmux server.
  5. Host-oriented remote refresh.

Refs #1753. Follow-up to #1756.

Summary by CodeRabbit

  • New Features
    • Added adaptive background refreshing (v2) to reduce status polling for unchanged off-screen sessions using a viewport-aware refresh gate.
    • Introduced the ui.adaptive_refresh_max_skips setting to control the adaptive skip ceiling, with 0 using defaults and negative values disabling the policy.
  • Performance
    • Improved session render snapshot updates by reusing existing snapshots when render state is unchanged.
    • Updated status polling behavior to respect visible-row budgets and promptly refresh when the viewport changes.
  • Tests
    • Added extensive unit coverage for adaptive refresh policy, ledger/budget behavior, and snapshot rebuild logic.

…ck (#1753)

The refresh tick did work proportional to ALL live sessions. #1756 fixed the
render half; this fixes the status sweep, which is the structurally wrong shape:
it re-derives state for sessions that provably did not change, using the most
expensive mechanism available.

Three facts compose badly. Only livePipeLRUCapacity (3) + attached sessions hold
a control pipe, so the sweep's existing "PipeManager saw no output for 5s" skip
covers ~4 rows out of 70. Without a pipe, CapturePane is a `tmux capture-pane`
subprocess serialized by the tmux server. And the most common steady state on a
large deck — a Claude session parked in hook "waiting" — takes the hook fast
path, which calls BackgroundWorkPending() and therefore captures the pane every
bgWorkCacheTTL (3s). At ~60 quiescent sessions that is tens of subprocesses per
sweep to re-derive statuses identical to the ones already on screen.

Adds an adaptive gate (internal/ui/refresh_policy.go). A per-session
fingerprint is built entirely from caches the sweep already refreshed once —
window_activity, pane title/command/dead, hook UpdatedAt, SSE UpdatedAt — so
building it for the whole deck is a handful of map lookups and spawns nothing.
UpdateStatus is skipped only when EVERY one of these holds: the row is
off-screen, the status is running/waiting/idle, the fingerprint is usable and
identical to the last polled one, and fewer than maxSkips consecutive skips have
happened. Any veto polls; nothing is ever skipped on absent information.

Unchanged window_activity as proof of "no new pane output" is not a new
assumption — Session.GetStatus already gates its CapturePane on exactly that
comparison, and UpdateStatus gates its idle tier on it. The gate is strictly
more conservative than both, because those skip for as long as activity is
unchanged while this one is bounded: after maxSkips consecutive skips a session
is polled regardless, so the time-based transitions inside UpdateStatus
(acknowledged -> idle, hook-freshness expiry, debounce confirmation) are delayed
by at most maxSkips sweeps rather than suppressed. In practice they are not
delayed at all: anything that actually happens moves the fingerprint on the very
next sweep (Stop hook -> hookAt, output -> activity, spinner -> title, exit ->
dead, vanished session -> unusable fingerprint -> forced poll).

Viewport publication is O(visible rows) — it walks only the viewport slice of
flatItems — and fails open: with no snapshot, or one older than 10s (event loop
suspended by tea.Exec or wedged), the gate is bypassed and every session is
polled, exactly as before.

Also generation-skips refreshSessionRenderSnapshot, which is called from six
sweep/tick paths and used to allocate and publish a fresh N-entry map every
time. It now compares first and returns without allocating or storing when
nothing changed. The per-state derivation moved into computeSessionRenderState
so the compare pass and the build pass cannot drift.

Kill switch: [ui] adaptive_refresh_max_skips. 0/unset = default 2 (poll every
quiescent off-screen session at least every 3rd sweep, ~6s); >0 = custom
ceiling; <0 = disable the policy entirely, byte-identical to pre-policy
behaviour.

docs/design/refresh-architecture-v2.md carries the full target architecture:
event-driven first (hook events already land via fsnotify but StatusFileWatcher
is constructed with a nil onChange, so the TUI polls a push pipeline instead of
listening to it), adaptive second (this slice), remote-ready third (host as the
unit of cost, one persistent connection and one bulk fingerprint query per
remote, so the Mac's cost is O(hosts) not O(sessions)) — plus the invariants any
future change to this layer must preserve.

Tests: pure unit coverage of every veto and both fail-open paths, the staleness
ceiling pattern, fingerprint evidence/no-evidence cases, viewport publication,
and the render-snapshot skip asserted by map identity. No tmux server, no HOME.

Committed by Ashesh Goplani
@github-actions

Copy link
Copy Markdown

👋 Thanks for opening this — a couple of intake fields would help the maintainer's validation pipeline (and any reviewer) understand it faster. Nothing here blocks you and nothing is closing: this is a heads-up, and editing the description re-runs this check automatically.

  • Missing required sections: ## What problem does this solve?, ## Why this change, ## User impact, ## AI disclosure, ## What actually bothered you, ## Checklist.
  • No AI-disclosure box is checked — pick exactly one (Human-written / AI-assisted / AI-authored). AI is welcome here with equal standing; this is a signal, not a filter.
  • ## What actually bothered you is empty — one real sentence of human intent is the one field intake cannot accept blank. If an agent opened this PR, quote what the human asked for.

The full field-level contract is in .github/INTAKE.md. AI-authored PRs are welcome here with equal standing.

@coderabbitai

coderabbitai Bot commented Jul 26, 2026

Copy link
Copy Markdown

Review Change Stack

📝 Walkthrough

Walkthrough

Adds configurable adaptive refresh skipping for unchanged sessions, with viewport-aware polling budgets, cache-derived fingerprints, refresh ledgers, fail-open visibility handling, and render snapshot reuse. Tests cover configuration, policy decisions, ledger behavior, evidence, wiring, and rendering.

Changes

Adaptive status refresh

Layer / File(s) Summary
Configuration and refresh ledger
internal/session/userconfig.go, internal/ui/refresh_policy.go
Adds adaptive refresh configuration, skip vetoes, fingerprint baselines, visible-row holds, polling budgets, and ledger maintenance.
Fingerprint evidence and viewport snapshots
internal/ui/refresh_policy.go, internal/session/watcher_seed_test_helper.go, internal/tmux/seed_test_helper.go
Combines tmux, hook, and SSE evidence into fingerprints and publishes fresh visible-session snapshots with fail-open behavior.
Home sweep and render integration
internal/ui/home.go
Wires adaptive state into initialization, foreground/background polling, viewport publication, pruning, invalidation, request sizing, and render snapshot reuse.
Policy, wiring, and render validation
internal/ui/*_test.go, internal/session/*_test.go
Validates configuration mapping, skip decisions, ledger transitions, cache evidence, viewport handling, Home wiring, and render rebuild behavior.

Estimated code review effort: 4 (Complex) | ~60 minutes

Sequence Diagram(s)

sequenceDiagram
  participant Home
  participant VisibleSnapshot
  participant TmuxCache
  participant RefreshLedger
  participant Session
  Home->>VisibleSnapshot: publish visible session IDs
  Home->>TmuxCache: read cached session and pane evidence
  Home->>RefreshLedger: evaluate fingerprint and polling budget
  alt poll required
    Home->>Session: call UpdateStatus
    Home->>RefreshLedger: record refreshed baseline
  else unchanged session
    RefreshLedger-->>Home: skip or hold refresh
  end
Loading

Possibly related PRs

🚥 Pre-merge checks | ✅ 5 | ❌ 2

❌ Failed checks (2 warnings)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 69.44% which is insufficient. The required threshold is 70.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
Remote_parity ⚠️ Warning Adaptive refresh only gates *session.Instance sweeps; changed code/tests never mention RemoteSession or add a skip explaining why. Add remote-session coverage, or an explicit t.Skip/comment documenting why this TUI change does not apply to ItemTypeRemoteSession rows.
✅ Passed checks (5 passed)
Check name Status Explanation
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Test_coverage_per_surface ✅ Passed PASS: The adaptive-refresh feature is covered by session-config and TUI tests, and no CLI/web/remote code consumes this knob, so no other surface is applicable.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title uses a valid Conventional Commits form and accurately summarizes the PR’s adaptive refresh and generation-skip changes.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch perf/refresh-tick-v2

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick comments (1)
internal/ui/refresh_policy_test.go (1)

286-332: 📐 Maintainability & Code Quality | 🔵 Trivial

No RemoteSession coverage or documented skip for adaptive-refresh viewport tests.

This test file changes TUI behavior in internal/ui/ (viewport publication for the adaptive sweep gate) but has no test exercising ItemTypeRemoteSession rows in publishVisibleSessions, nor an explicit t.Skip documenting why it's excluded.

The adaptive sweep only iterates local *session.Instance in backgroundStatusUpdate, so RemoteSession rows are likely structurally out of scope for the skip decision (similar to the prior finding that RemoteSession is structurally inapplicable to fork-dispatch helpers) — but the viewport/visible-set publication itself walks flatItems, which can contain ItemTypeRemoteSession rows, so a short comment or explicit skip clarifying the exclusion would satisfy the guideline.

As per coding guidelines, internal/ui/**/*_test.go: "When a PR changes TUI behavior in internal/ui/ or cmd/agent-deck/, verify it also handles RemoteSession or include an explicit t.Skip documenting why." Based on learnings, RemoteSession is treated as structurally inapplicable to similar internal/ui dispatch/gating helpers where the local Instance is required.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@internal/ui/refresh_policy_test.go` around lines 286 - 332, The
adaptive-refresh viewport tests need explicit RemoteSession coverage or a
documented exclusion. In TestPublishVisibleSessions_CoversViewportAndCursor, add
a focused RemoteSession case if publishVisibleSessions should include those
rows; otherwise add an explicit t.Skip explaining that RemoteSession rows are
outside the local *session.Instance sweep scope while flatItems publication
remains structurally covered.

Sources: Coding guidelines, Learnings

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Nitpick comments:
In `@internal/ui/refresh_policy_test.go`:
- Around line 286-332: The adaptive-refresh viewport tests need explicit
RemoteSession coverage or a documented exclusion. In
TestPublishVisibleSessions_CoversViewportAndCursor, add a focused RemoteSession
case if publishVisibleSessions should include those rows; otherwise add an
explicit t.Skip explaining that RemoteSession rows are outside the local
*session.Instance sweep scope while flatItems publication remains structurally
covered.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: 59a0df42-4d51-42aa-9643-27673113187b

📥 Commits

Reviewing files that changed from the base of the PR and between 672f8a2 and 9345594.

⛔ Files ignored due to path filters (1)
  • docs/design/refresh-architecture-v2.md is excluded by !**/*.md
📒 Files selected for processing (5)
  • internal/session/userconfig.go
  • internal/tmux/seed_test_helper.go
  • internal/ui/home.go
  • internal/ui/refresh_policy.go
  • internal/ui/refresh_policy_test.go

…ts (#1753)

Second pass on the adaptive refresh policy, closing the three review items on
the PR and the new bar recorded on the issue (the live-fleet diagnostic: lag
reproduces with a large group EXPANDED, where visible ≈ fleet and an
off-screen-only skip degenerates to refreshing everything).

Visible rows now get the same fingerprint treatment, shaped for what the user
sees. A visible row with a settled status and an unchanged fingerprint is held
under the same maxSkips ceiling as an off-screen row (refreshLedger.holdVisible),
so an expanded quiescent group costs ~zero. Visible rows that DO need a poll
compete for a per-sweep budget (visiblePollBudgetPerSweep = 10, the sweep's
worker-pool width); deferred rows age a starvation counter and are admitted
first on later sweeps, so the budget cycles round-robin and no row starves. The
cursor row is always admitted — it drives the preview pane. The incremental
visible-first pass in processStatusUpdate, which used to poll EVERY visible row
per pass, now holds steady rows read-only (heldSteady — no ceiling, because the
background sweep owns freshness) and budgets the rest the same way. Group
expand republishes the viewport immediately and is otherwise only a flat-list
rebuild: rows render from the existing snapshot and fill in amortized, with no
per-row tmux work between the keypress and the first frame.

The config knob is now clamped: UISettings.GetAdaptiveRefreshMaxSkips resolves
[ui] adaptive_refresh_max_skips with the GetRemoteSessionRefreshSecs pattern —
0/unset → 2, negative → kill switch, positive clamped to 30 (it previously
accepted 100000 verbatim, a ~55h staleness ceiling). The knob resolves once at
TUI construction, so changes — including the kill switch — need a TUI restart;
documented in the struct comment and the design doc.

The safety wiring is now itself under test, not just the policy functions.
internal/ui/refresh_wiring_test.go drives the real backgroundStatusUpdate on a
constructed Home and pins: (a) a nil / wrong-type / >10s-stale viewport
snapshot forces maxSkips=0 so every row polls — asserted via a seeded ledger
entry that must be left byte-identical, which fails if the fail-open line is
deleted because entering the gate always rewrites the entry; (b) a fresh
snapshot consults the gate (positive control); (c) a due visible row is
admitted, rebased, and polled; (d) refreshAttachedSessionStatus drops the
ledger baseline on attach-return — fails if the forget() line is deleted.
tmux.SeedServerAliveForTest pins the server-alive probe so these run
deterministically with or without a tmux server. The hook/SSE fingerprint
inputs (hookAt/sseAt) — what makes "real transitions are not delayed" true —
get direct coverage via new watcher test seams.

Measured on the sandboxed #1753 harness (throwaway HOME, isolated tmux socket,
60 synthetic sleep sessions, 220x50, 240 j/k events; same binary, kill switch
vs default): pre-policy polls 60/60 sessions every sweep (zero skip lines in
~90 sweeps, one 639ms slow_status_loop); with the policy the median sweep
polls 10 of 60 (mean 16.5, adaptive_skipped mean 32.2, visible_deferred mean
11.3) — an 83% median reduction in UpdateStatus calls with no slow sweeps.
Both passes produced identical status transitions (120 each) and no flicker.

Committed by Ashesh Goplani
@asheshgoplani

Copy link
Copy Markdown
Owner Author

Second pass pushed — all three review items closed, plus the new bar recorded on #1753 (visible-row budget + group-expand as a trigger case) since the live-fleet diagnostic landed mid-review. Head: 458dbcaeba5766153ff46362330f80299ba69795.

1. The safety wiring is now under test (not just the policy functions)

internal/ui/refresh_wiring_test.go constructs a Home and drives the real backgroundStatusUpdate / refreshAttachedSessionStatus. Which deletion each test catches, and why:

  • Deleting the fail-open maxSkips = 0 line (no fresh viewport snapshot ⇒ poll everything) → TestBackgroundStatusUpdate_FailsOpenWithoutFreshViewportSnapshot goes red. Mechanism: the test seeds a ledger entry at skips=1 and asserts it is left byte-identical after a sweep with a nil / wrong-type / >10s-stale snapshot (all three variants). Entering the gate always rewrites the consulted entry — decide() records every outcome, and inside a hermetic sweep the fingerprint is unusable, so the gate would poll and rebase the entry (skips → 0, baseline replaced). With the line intact the gate is bypassed, the entry survives untouched, and both seeded instances' statuses observably leave running (every row polled). So the assertion pair distinguishes "polled because failed open" from "polled because the gate vetoed" — only the former leaves the ledger untouched.
  • TestBackgroundStatusUpdate_ConsultsGateWithFreshSnapshot is the positive control: with a fresh snapshot the entry must be rewritten. This pins the gate block itself, so the fail-open test can't be satisfied trivially by the gate not existing.
  • Deleting h.refreshLedger.forget(inst.ID) in refreshAttachedSessionStatusTestRefreshAttachedSessionStatus_ForgetsLedgerBaseline goes red: it seeds a baseline and asserts the entry is gone after attach-return.
  • Bonus wiring pin for the new visible path: TestBackgroundStatusUpdate_VisibleDueRowIsAdmittedAndRebased fails if the budget-admission block is deleted (a due visible row would then never be polled and the seeded entry would survive).

Per the ground rules I did not run go test on this host; the deletion-detection argument above is by construction (every ledger mutation path is exercised), and CI runs the suite.

2. The knob is clamped

UISettings.GetAdaptiveRefreshMaxSkips (internal/session/userconfig.go), same shape as GetRemoteSessionRefreshSecs: 0/unset → default 2; negative → 0 (kill switch); positive clamped to MaxAdaptiveRefreshMaxSkips = 30100000 now resolves to 30 instead of a ~55-hour staleness ceiling. refresh_policy.go's unclamped resolver is deleted and NewHome reads through the new accessor. Docs (struct comment + design doc §3.5) now state the knob resolves once at TUI construction, so any change — including the kill switch — needs a TUI restart. Table-driven test in userconfig_adaptive_refresh_test.go.

3. Measured, not predicted

Ran the sandboxed #1753 harness on this branch (throwaway HOME, blank XDG, own profile, own tmux socket with same-socket teardown, 60 synthetic sleep sessions, 220×50, 60s warmup + 240 single-key j/k + 60s settle). Same binary both passes: before = kill switch -1 (byte-identical pre-policy), after = default. From the idle_sessions_skipped counters this PR added:

before after
UpdateStatus calls/sweep (median) 60 of 60 10 of 60
calls/sweep (mean) 60 16.5
adaptive_skipped mean 0 32.2
visible_deferred mean 0 11.3
slow_status_loop (>500ms) 1 (639ms) 0
status transitions 120 120 (identical, 0 flicker)

~83% median reduction in UpdateStatus calls, identical status convergence. The PR body's "Expected effect" section is replaced with these numbers. Notably, at 220×50 ~44 of the 60 rows are visible, so this run measures exactly the group-expanded shape from the new diagnostic — the original off-screen-only gate would have skipped at most ~16 rows here.

The hook/SSE fingerprint inputs also got the requested coverage: TestFingerprintSession_HookAndSSEInputsMoveTheFingerprint pins that a delivered hook (or SSE sample) moves hookAt/sseAt and breaks the match even when tmux evidence is unchanged — the load-bearing half of the "real transitions are not delayed" argument — via new watcher test seams in internal/session.

New since the verdict: visible rows + group expand (#1753 diagnostic)

  • Visible rows get the same fingerprint hold under the same ceiling (holdVisible), so an expanded quiescent group costs ~zero.
  • Visible rows that do need a poll are budgeted: at most 10/sweep (the worker-pool width), round-robin via a starvation counter (admitVisiblePolls), cursor row always admitted. Same treatment in processStatusUpdate's visible-first pass (heldSteady, read-only + budget), which previously polled every visible row per pass.
  • Group expand republishes the viewport immediately and is otherwise only a flat-list rebuild — rows render from the existing snapshot and fill in amortized; no per-row tmux work between keypress and first frame.
  • Kill switch and fail-open disable hold and budget alike (pre-policy byte-identical).

Design doc gained §3.2b covering all of this. NOT merging — awaiting re-review at the new head.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick comments (1)
internal/ui/home.go (1)

8017-8022: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Deduplicate the group-expand refresh block.

The zero-argument publishVisibleSessions() calls are valid because the method is variadic, but the double-click, Enter, and Tab/right group paths all repeat the same toggle/rebuild cursor-publish/save logic. Extract a small helper and parameterize per-toggle semantics instead of keeping the blocks in sync manually.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@internal/ui/home.go` around lines 8017 - 8022, Deduplicate the repeated
group-expand refresh logic in the double-click, Enter, and Tab/right paths by
extracting a small helper near the relevant UI handlers. Parameterize the helper
with each toggle’s distinct semantics, while centralizing the shared toggle,
cursor rebuild, immediate publishVisibleSessions(), and save behavior; update
all three paths to use it.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Nitpick comments:
In `@internal/ui/home.go`:
- Around line 8017-8022: Deduplicate the repeated group-expand refresh logic in
the double-click, Enter, and Tab/right paths by extracting a small helper near
the relevant UI handlers. Parameterize the helper with each toggle’s distinct
semantics, while centralizing the shared toggle, cursor rebuild, immediate
publishVisibleSessions(), and save behavior; update all three paths to use it.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: 28796a04-8ef7-4fca-8b64-c78240849700

📥 Commits

Reviewing files that changed from the base of the PR and between 9345594 and 458dbca.

⛔ Files ignored due to path filters (1)
  • docs/design/refresh-architecture-v2.md is excluded by !**/*.md
📒 Files selected for processing (8)
  • internal/session/userconfig.go
  • internal/session/userconfig_adaptive_refresh_test.go
  • internal/session/watcher_seed_test_helper.go
  • internal/tmux/seed_test_helper.go
  • internal/ui/home.go
  • internal/ui/refresh_policy.go
  • internal/ui/refresh_policy_test.go
  • internal/ui/refresh_wiring_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
  • internal/ui/refresh_policy.go

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant