From 45da6a5488295e717a6a7d07745325e14b57c089 Mon Sep 17 00:00:00 2001 From: hellices Date: Thu, 6 Aug 2026 21:38:07 +0900 Subject: [PATCH 1/3] perf: memoise table rows so a repaint costs only what changed MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Every watch event triggered a full-table repaint whose cost was proportional to the total row count, not to the number of objects that changed, and it ran synchronously on the event loop — so keypresses queued behind it. At 10,000 objects a single repaint cost ~170 ms and 10 events/s pushed cursor movement past 2.5 s; at 50,000 objects one repaint cost ~1.1 s. Watch summaries are frozen dataclasses replaced wholesale, so a row whose summary object is unchanged provably renders identical cells. Four changes: - `ResourceTable._row_memo` keyed by summary identity plus a `stamp` covering the inputs that are *not* carried by the summary (the age string, and the metrics sample on the pods view). A hit re-emits the memoised row untouched. The memo is invalidated when the cell-set shape changes (kind, all-namespaces, custom view) and pruned amortised as row names churn. - The in-place diff compares against `_emitted` — the cells this widget last put into the table — instead of `DataTable.get_row()`, which re-derived `ordered_columns` on every call. A memo hit is settled by one identity check; `get_row` stays as the fallback when `_emitted` has no record. - One `datetime.now(UTC)` per repaint threaded into every `age()` call, replacing a clock read plus RFC 3339 reparse per row per repaint. Rows in one repaint can no longer straddle a second boundary either. - Cell builders skip rebuilding memo-hit rows entirely. Measured on the same fixtures as issue #208 (pods, all-namespaces): | rows | repaint before | repaint after | cell build before | after | |--------|----------------|---------------|-------------------|---------| | 1,000 | 11.40 ms | 1.04 ms | 6.34 ms | 0.63 ms| | 10,000 | 170.05 ms | 11.46 ms | 139.17 ms | 7.05 ms| | 50,000 | 1,102.36 ms | 80.12 ms | 780.61 ms | 42.82 ms| Keypress latency under churn (30 samples, cursor-down), 10 events/s: | rows | before | after | |--------|----------|---------| | 10,000 | 2,551 ms | 100 ms| | 50,000 | 3,364 ms | 2,178 ms| Cold first build is unchanged (it has no memo to hit) and remains the dominant cost at 50,000 rows; that is a separate bootstrap problem. Fixes #208 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- src/korvid/ui/widgets/resource_table.py | 172 ++++++++++++++++++++---- tests/ui/test_table_row_memo.py | 159 ++++++++++++++++++++++ 2 files changed, 302 insertions(+), 29 deletions(-) create mode 100644 tests/ui/test_table_row_memo.py diff --git a/src/korvid/ui/widgets/resource_table.py b/src/korvid/ui/widgets/resource_table.py index 8e124ebc..2607fb79 100644 --- a/src/korvid/ui/widgets/resource_table.py +++ b/src/korvid/ui/widgets/resource_table.py @@ -3,7 +3,8 @@ from __future__ import annotations from collections.abc import Callable -from typing import cast +from datetime import UTC, datetime +from typing import Final, cast from rich.cells import cell_len from rich.text import Text @@ -34,6 +35,10 @@ #: Looks up live metrics for (namespace, name); None disables the join. MetricsLookup = Callable[[str, str], PodMetrics | None] +#: `_emit_row(stamp=...)` sentinel: this row opts out of the memo. Distinct +#: from `None`, which is a legitimate stamp (a row with no volatile inputs). +_NO_STAMP: Final = object() + _POD_COLS = ( "NAME", "READY", @@ -350,6 +355,16 @@ def on_mount(self) -> None: self._last_sort = None self._active_view = None self._pending_rows: list[tuple[str, list[str | Text]]] = [] + #: Per-row cell memo (issue #208): row key -> (summary, stamp, row). + #: Keeps a repaint proportional to the rows that actually changed. + self._row_memo: dict[str, tuple[Summary, object, tuple[str, list[str | Text]]]] = {} + #: Cell-set shape the memo was built for; a change invalidates it. + self._memo_signature: tuple[str, bool, ViewConfig | None] | None = None + #: Cells currently in the table, so the diff never has to read them + #: back out of the DataTable one `get_row` at a time. + self._emitted: dict[str, list[str | Text]] = {} + #: Single clock reading per repaint; see `show()`. + self._render_now: datetime = datetime.now(UTC) def show( self, @@ -380,6 +395,17 @@ def show( self._active_view, ) restore = self._cursor_snapshot() if same_view else None + # One clock reading per repaint: every AGE cell is derived from the + # same instant, and the memo below can compare age strings without + # each row racing its own `datetime.now()`. + self._render_now = datetime.now(UTC) + # The memo holds finished cell lists, so it is only valid while the + # cell set has the same shape. Sort is deliberately absent: it + # reorders rows and decorates headers, it does not change any cell. + signature = (kind, all_namespaces, view) + if signature != self._memo_signature: + self._row_memo.clear() + self._memo_signature = signature # Build the desired rows first (the builders only fill # `_pending_rows`), so a pure data refresh can be diffed against the # current table instead of clearing and rebuilding it. @@ -389,6 +415,7 @@ def show( kind, rows, all_namespaces=all_namespaces, pattern=pattern, metrics=metrics, sort=sort ) pending, self._pending_rows = self._pending_rows, [] + self._prune_memo(pending) if same_view and sort == self._last_sort and self._apply_in_place(pending): # Nothing was cleared, so the scroll offset never moved; only # the cursor may have slid when rows above it were removed — @@ -431,6 +458,7 @@ def show( self.clear() for key, cells in pending: self.add_row(*cells, key=key) + self._emitted = dict(pending) if restore is not None: # A background refresh must not scroll the cursor back into # view — the viewport restore below keeps the user's position. @@ -488,6 +516,14 @@ def _apply_in_place(self, pending: list[tuple[str, list[str | Text]]]) -> bool: place; returns False when the update needs the rebuild path (see `_in_place_plan` for the eligibility rules). + The comparison runs against `_emitted` — the cells this widget last + put into the table — rather than `DataTable.get_row()`, which re-derives + `ordered_columns` on every call and would make an untouched repaint + cost one lookup plus a full cell comparison per row (issue #208). + A memo hit re-emits the *same* cell list, so an unchanged row is + settled by one identity check. `get_row` remains the fallback whenever + `_emitted` has no record of a row. + Width updates are requested only for cells wider than their column: `update_width=True` is not grow-only — a narrower replacement rescans and *shrinks* the column, shifting the layout this path must keep @@ -502,16 +538,33 @@ def _apply_in_place(self, pending: list[tuple[str, list[str | Text]]]) -> bool: self.remove_row(key) columns = self.ordered_columns for key, cells in pending: - if key in current_set: - old_cells = self.get_row(key) - for column, old_cell, new_cell in zip(columns, old_cells, cells, strict=True): - if not _cells_equal(old_cell, new_cell): - grew = _cell_width(new_cell) > column.content_width - self.update_cell(key, column.key, new_cell, update_width=grew) - else: + if key not in current_set: self.add_row(*cells, key=key) + continue + old_cells = self._emitted.get(key) + if old_cells is cells: + continue # memo hit: same list object, nothing can have changed + if old_cells is None: + old_cells = self.get_row(key) + for column, old_cell, new_cell in zip(columns, old_cells, cells, strict=True): + if not _cells_equal(old_cell, new_cell): + grew = _cell_width(new_cell) > column.content_width + self.update_cell(key, column.key, new_cell, update_width=grew) + self._emitted = dict(pending) return True + def _prune_memo(self, pending: list[tuple[str, list[str | Text]]]) -> None: + """Drop memo entries for rows no longer rendered. + + Left alone the memo would grow for the lifetime of the session as pod + names churn. Pruning is amortised: it only runs once the memo has + drifted well past the rendered set, so the common repaint stays free. + """ + if len(self._row_memo) <= 4 * len(pending) + 1024: + return + live = {key for key, _ in pending} + self._row_memo = {key: entry for key, entry in self._row_memo.items() if key in live} + def _cursor_snapshot(self) -> tuple[str, int] | None: """(row key, row index) under the cursor, or None on an empty table.""" if self.row_count == 0 or self.cursor_row < 0: @@ -531,7 +584,14 @@ def _restore_cursor(self, key: str, index: int, *, scroll: bool = True) -> None: row = min(index, self.row_count - 1) self.move_cursor(row=row, animate=False, scroll=scroll) - def _emit_row(self, obj: Summary, cells: list[str | Text], *, all_namespaces: bool) -> None: + def _emit_row( + self, + obj: Summary, + cells: list[str | Text], + *, + all_namespaces: bool, + stamp: object = _NO_STAMP, + ) -> None: """Finish one row: apply the custom view (issue #45), prepend the namespace in all-namespaces mode, and buffer it keyed by ns/name for `show()` to diff or add. @@ -539,6 +599,12 @@ def _emit_row(self, obj: Summary, cells: list[str | Text], *, all_namespaces: bo *cells* is the kind's default cell list without the namespace. Rows whose summaries carry fewer custom values than configured (e.g. seeded before the config existed) pad with ``. + + *stamp* is everything the cells depend on that is *not* carried by the + frozen summary itself (the age string, plus the metrics sample on the + pods view). It is memoised with the row so `_reuse_row` can prove the + cells would come out identical; `_NO_STAMP` opts a caller out of the + memo entirely. """ view = self._active_view if view is not None: @@ -549,7 +615,23 @@ def _emit_row(self, obj: Summary, cells: list[str | Text], *, all_namespaces: bo cells = [cells[0], *extras] if view.replace else [*cells, *extras] if all_namespaces: cells.insert(0, obj.namespace) - self._pending_rows.append((f"{obj.namespace}/{obj.name}", cells)) + row = (f"{obj.namespace}/{obj.name}", cells) + self._pending_rows.append(row) + if stamp is not _NO_STAMP: + self._row_memo[row[0]] = (obj, stamp, row) + + def _reuse_row(self, obj: Summary, stamp: object) -> bool: + """Re-emit *obj*'s memoised row when nothing feeding its cells changed. + + Watch events replace the whole frozen summary, so an identity hit + proves every summary-derived cell is unchanged; *stamp* covers the + rest. Returns False when the row must be rebuilt. + """ + entry = self._row_memo.get(f"{obj.namespace}/{obj.name}") + if entry is None or entry[0] is not obj or entry[1] != stamp: + return False + self._pending_rows.append(entry[2]) + return True def _render_rows( self, @@ -621,10 +703,17 @@ def _add_pod_rows( pods = cast(list[PodSummary], rows) if not presorted: pods = sorted(pods, key=_pod_sort_key) + now = self._render_now for pod in pods: if pattern and pattern.lower() not in pod.name.lower(): continue usage = metrics(pod.namespace, pod.name) if metrics is not None else None + age = pod.age(now) + # Everything else on this row is derived from the frozen summary, + # so the live metrics sample and the age are the whole stamp. + stamp = (age, usage) + if self._reuse_row(pod, stamp): + continue cells: list[str | Text] = [ pod.name, _ready_cell(pod.ready), @@ -634,10 +723,10 @@ def _add_pod_rows( f"{pod.cpu_request}/{pod.cpu_limit}", f"{pod.mem_request}/{pod.mem_limit}", Text(pod.qos, style=_QOS_STYLE.get(pod.qos, "dim")), - pod.age(), + age, pod.node or "-", ] - self._emit_row(pod, cells, all_namespaces=all_namespaces) + self._emit_row(pod, cells, all_namespaces=all_namespaces, stamp=stamp) def _add_replicaset_rows( self, rows: list[Summary], *, all_namespaces: bool, pattern: str, presorted: bool = False @@ -664,18 +753,23 @@ def _add_replicaset_rows( if pattern and pattern.lower() not in obj.name.lower(): continue if isinstance(obj, ReplicaSetSummary): + age = obj.age(self._render_now) + if self._reuse_row(obj, age): + continue cells: list[str | Text] = [ obj.name, obj.revision, str(obj.desired), str(obj.current), _ready_cell(obj.ready), - obj.age(), + age, ] else: - age = obj.age() if isinstance(obj, GenericSummary) else "" + age = obj.age(self._render_now) if isinstance(obj, GenericSummary) else "" + if self._reuse_row(obj, age): + continue cells = [obj.name, "", "", "", "", age] - self._emit_row(obj, cells, all_namespaces=all_namespaces) + self._emit_row(obj, cells, all_namespaces=all_namespaces, stamp=age) def _add_helm_release_rows( self, rows: list[Summary], *, all_namespaces: bool, pattern: str, presorted: bool = False @@ -686,15 +780,18 @@ def _add_helm_release_rows( for rel in releases: if pattern and pattern.lower() not in rel.name.lower(): continue + age = rel.age(self._render_now) + if self._reuse_row(rel, age): + continue cells: list[str | Text] = [ rel.name, str(rel.revision), _helm_status_cell(rel.status), rel.chart, rel.app_version, - rel.age(), + age, ] - self._emit_row(rel, cells, all_namespaces=all_namespaces) + self._emit_row(rel, cells, all_namespaces=all_namespaces, stamp=age) def _add_helm_revision_rows( self, rows: list[Summary], *, all_namespaces: bool, pattern: str, presorted: bool = False @@ -706,6 +803,9 @@ def _add_helm_revision_rows( for rev in revisions: if pattern and pattern.lower() not in rev.name.lower(): continue + age = rev.age(self._render_now) + if self._reuse_row(rev, age): + continue cells: list[str | Text] = [ rev.name, str(rev.revision), @@ -713,9 +813,9 @@ def _add_helm_revision_rows( rev.chart, rev.app_version, rev.description, - rev.age(), + age, ] - self._emit_row(rev, cells, all_namespaces=all_namespaces) + self._emit_row(rev, cells, all_namespaces=all_namespaces, stamp=age) def _add_fallback_rows( self, @@ -734,9 +834,11 @@ def _add_fallback_rows( for obj in rows: if pattern and pattern.lower() not in obj.name.lower(): continue - age = obj.age() if isinstance(obj, GenericSummary) else "" + age = obj.age(self._render_now) if isinstance(obj, GenericSummary) else "" + if self._reuse_row(obj, age): + continue cells: list[str | Text] = [obj.name, *[""] * (width - 2), age] - self._emit_row(obj, cells, all_namespaces=all_namespaces) + self._emit_row(obj, cells, all_namespaces=all_namespaces, stamp=age) def _add_package_rows( self, rows: list[Summary], *, all_namespaces: bool, pattern: str, presorted: bool = False @@ -747,15 +849,18 @@ def _add_package_rows( for pkg in packages: if pattern and pattern.lower() not in pkg.name.lower(): continue + age = pkg.age(self._render_now) + if self._reuse_row(pkg, age): + continue cells: list[str | Text] = [ pkg.name, pkg.catalog or "-", pkg.default_channel or "-", ",".join(pkg.channels) or "-", pkg.description or "-", - pkg.age(), + age, ] - self._emit_row(pkg, cells, all_namespaces=all_namespaces) + self._emit_row(pkg, cells, all_namespaces=all_namespaces, stamp=age) fallbacks = [r for r in rows if not isinstance(r, PackageManifestSummary)] self._add_fallback_rows( fallbacks, @@ -774,15 +879,18 @@ def _add_subscription_rows( for sub in subs: if pattern and pattern.lower() not in sub.name.lower(): continue + age = sub.age(self._render_now) + if self._reuse_row(sub, age): + continue cells: list[str | Text] = [ sub.name, sub.channel or "-", sub.source or "-", sub.installed_csv or "-", sub.state or "-", - sub.age(), + age, ] - self._emit_row(sub, cells, all_namespaces=all_namespaces) + self._emit_row(sub, cells, all_namespaces=all_namespaces, stamp=age) fallbacks = [r for r in rows if not isinstance(r, OLMSubscriptionSummary)] self._add_fallback_rows( fallbacks, @@ -801,14 +909,17 @@ def _add_csv_rows( for csv in csvs: if pattern and pattern.lower() not in csv.name.lower(): continue + age = csv.age(self._render_now) + if self._reuse_row(csv, age): + continue cells: list[str | Text] = [ csv.name, csv.display_name or "-", csv.version or "-", _csv_phase_cell(csv.phase), - csv.age(), + age, ] - self._emit_row(csv, cells, all_namespaces=all_namespaces) + self._emit_row(csv, cells, all_namespaces=all_namespaces, stamp=age) fallbacks = [r for r in rows if not isinstance(r, CSVSummary)] self._add_fallback_rows( fallbacks, @@ -827,5 +938,8 @@ def _add_generic_rows( for obj in generics: if pattern and pattern.lower() not in obj.name.lower(): continue - cells: list[str | Text] = [obj.name, obj.age()] - self._emit_row(obj, cells, all_namespaces=all_namespaces) + age = obj.age(self._render_now) + if self._reuse_row(obj, age): + continue + cells: list[str | Text] = [obj.name, age] + self._emit_row(obj, cells, all_namespaces=all_namespaces, stamp=age) diff --git a/tests/ui/test_table_row_memo.py b/tests/ui/test_table_row_memo.py new file mode 100644 index 00000000..231e98ae --- /dev/null +++ b/tests/ui/test_table_row_memo.py @@ -0,0 +1,159 @@ +"""Row-cell memo (issue #208): a repaint must cost work proportional to the +rows that actually changed, not to the total row count. + +Watch summaries are frozen dataclasses replaced wholesale, so a row whose +summary object is unchanged provably renders identical cells — its cells are +reused instead of rebuilt, and the in-place diff skips it without asking the +DataTable what it already holds. +""" + +from __future__ import annotations + +from typing import Any + +from korvid.core.store import Summary +from korvid.k8s.metrics import PodMetrics +from korvid.ui.widgets.resource_table import ResourceTable + +from .test_app import _pod, make_app +from .waits import until + + +def _spy_emit(table: ResourceTable) -> list[str]: + """Record the row key of every row whose cells are actually rebuilt.""" + built: list[str] = [] + original = table._emit_row + + def spy(obj: Any, cells: Any, **kwargs: Any) -> Any: + built.append(f"{obj.namespace}/{obj.name}") + return original(obj, cells, **kwargs) + + table._emit_row = spy # type: ignore[method-assign] # test spy + return built + + +def _spy_get_row(table: ResourceTable) -> list[str]: + """Record the diff's own `get_row` lookups. + + Textual calls `get_row` with a `RowKey` while painting; only the diff + looks rows up by their string key, so filtering on `str` isolates it. + """ + looked_up: list[str] = [] + original = table.get_row + + def spy(row_key: Any) -> Any: + if isinstance(row_key, str): + looked_up.append(row_key) + return original(row_key) + + table.get_row = spy # type: ignore[method-assign] # test spy + return looked_up + + +async def test_repaint_rebuilds_only_the_changed_row() -> None: + """One MODIFIED pod must rebuild one row's cells, not every row's.""" + app = make_app([_pod("alpha"), _pod("beta"), _pod("gamma")]) + async with app.run_test() as pilot: + table = app.query_one(ResourceTable) + await until(pilot, lambda: table.row_count == 3, label="pods loaded") + built = _spy_emit(table) + app.store.apply_event("pods", "default", "MODIFIED", _pod("beta", phase="Failed")) + await until( + pilot, + lambda: str(table.get_row("default/beta")[2]) == "Failed", + label="phase cell updated", + ) + assert built == ["default/beta"] + + +async def test_unchanged_repaint_rebuilds_nothing() -> None: + """A repaint with the same summary objects must not rebuild any cells.""" + app = make_app([_pod("alpha"), _pod("beta")]) + async with app.run_test() as pilot: + table = app.query_one(ResourceTable) + await until(pilot, lambda: table.row_count == 2, label="pods loaded") + built = _spy_emit(table) + table.show("pods", app.store.get("pods", "default"), all_namespaces=False, pattern="") + assert built == [] + + +async def test_unchanged_rows_are_not_read_back_from_the_datatable() -> None: + """The diff must settle rows from what this widget last emitted, never by + reading them back out of the DataTable one `get_row` at a time.""" + app = make_app([_pod("alpha"), _pod("beta"), _pod("gamma")]) + async with app.run_test() as pilot: + table = app.query_one(ResourceTable) + await until(pilot, lambda: table.row_count == 3, label="pods loaded") + read_row = table.get_row + looked_up = _spy_get_row(table) + app.store.apply_event("pods", "default", "MODIFIED", _pod("beta", phase="Failed")) + await until( + pilot, + lambda: str(read_row("default/beta")[2]) == "Failed", + label="phase cell updated", + ) + assert looked_up == [] + + +async def test_changed_metrics_rebuild_the_row_without_a_new_summary() -> None: + """Usage cells come from the metrics sample, not the summary: a new sample + for the same pod object must still re-render that row.""" + samples: dict[tuple[str, str], PodMetrics] = {} + + def lookup(namespace: str, name: str) -> PodMetrics | None: + return samples.get((namespace, name)) + + app = make_app([_pod("alpha")]) + async with app.run_test() as pilot: + table = app.query_one(ResourceTable) + await until(pilot, lambda: table.row_count == 1, label="pods loaded") + rows = app.store.get("pods", "default") + table.show("pods", rows, all_namespaces=False, pattern="", metrics=lookup) + built = _spy_emit(table) + samples[("default", "alpha")] = PodMetrics( + name="alpha", namespace="default", cpu_cores=0.5, memory_bytes=1024 * 1024 + ) + table.show("pods", rows, all_namespaces=False, pattern="", metrics=lookup) + assert built == ["default/alpha"] + assert str(table.get_row("default/alpha")[4]) != "-" + + +async def test_view_signature_change_discards_the_memo() -> None: + """Switching to the all-namespaces column set must rebuild every row: the + memoised cells were built without the NAMESPACE column.""" + app = make_app([_pod("alpha"), _pod("beta")]) + async with app.run_test() as pilot: + table = app.query_one(ResourceTable) + await until(pilot, lambda: table.row_count == 2, label="pods loaded") + built = _spy_emit(table) + table.show("pods", app.store.get("pods", "default"), all_namespaces=True, pattern="") + assert built == ["default/alpha", "default/beta"] + assert str(table.get_row("default/alpha")[0]) == "default" + + +async def test_age_uses_one_clock_reading_per_repaint() -> None: + """Every row in a repaint is aged against the same instant, so a repaint + that straddles a second boundary cannot report inconsistent ages.""" + import korvid.k8s.models as models + + app = make_app([_pod("alpha"), _pod("beta"), _pod("gamma")]) + async with app.run_test() as pilot: + table = app.query_one(ResourceTable) + await until(pilot, lambda: table.row_count == 3, label="pods loaded") + original = models.format_age + seen: list[Any] = [] + + def counting_format_age(value: str, now: Any = None) -> str: + seen.append(now) + return original(value, now=now) + + models.format_age = counting_format_age # type: ignore[assignment] # test spy + try: + # Fresh summary objects: identity differs, so every row rebuilds. + fresh: list[Summary] = [_pod("alpha"), _pod("beta"), _pod("gamma")] + table.show("pods", fresh, all_namespaces=False, pattern="") + finally: + models.format_age = original + assert len(seen) >= 3, seen + assert all(now is not None for now in seen), seen + assert len(set(seen)) == 1, seen From f89b2e4c1603a9c1cdf75840d673480ef75c0527 Mon Sep 17 00:00:00 2001 From: hellices Date: Thu, 6 Aug 2026 22:04:25 +0900 Subject: [PATCH 2/3] perf: keep replace-view rows out of the volatile stamp MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review of #209: with a `replace: true` custom view (issue #45), `_emit_row` keeps only NAME plus the configured custom values — both carried by the frozen summary — and discards the built-in AGE and metrics cells. The stamp still carried them, so a minute rollover or a metrics poll invalidated the memo and rebuilt cells nobody can see. Route every stamp through `_stamp()`, which collapses to `None` under a replace view because nothing volatile survives into the emitted row. Test: `test_replace_view_rows_ignore_hidden_volatile_cells` configures a replace view, publishes a new metrics sample for an unchanged pod object, and asserts no row is rebuilt while the rendered row stays `[NAME, TEAM]`. RED against 45da6a5 (the row rebuilt), GREEN now. Repaint cost is unchanged: 0.95 / 11.21 / 82.15 ms at 1k / 10k / 50k. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- src/korvid/ui/widgets/resource_table.py | 59 +++++++++++++++++-------- tests/ui/test_table_row_memo.py | 32 ++++++++++++++ 2 files changed, 73 insertions(+), 18 deletions(-) diff --git a/src/korvid/ui/widgets/resource_table.py b/src/korvid/ui/widgets/resource_table.py index 2607fb79..1bd5d5aa 100644 --- a/src/korvid/ui/widgets/resource_table.py +++ b/src/korvid/ui/widgets/resource_table.py @@ -633,6 +633,20 @@ def _reuse_row(self, obj: Summary, stamp: object) -> bool: self._pending_rows.append(entry[2]) return True + def _stamp(self, volatile: object) -> object: + """The volatile inputs that actually reach the emitted row. + + A `replace: true` custom view (issue #45) keeps only NAME plus the + configured custom values, all of which come from the frozen summary — + so the AGE and metrics cells it discards must not be allowed to + invalidate the memo, or a minute rollover or metrics poll would rebuild + cells nobody can see. + """ + view = self._active_view + if view is not None and view.replace: + return None + return volatile + def _render_rows( self, kind: str, @@ -711,7 +725,7 @@ def _add_pod_rows( age = pod.age(now) # Everything else on this row is derived from the frozen summary, # so the live metrics sample and the age are the whole stamp. - stamp = (age, usage) + stamp = self._stamp((age, usage)) if self._reuse_row(pod, stamp): continue cells: list[str | Text] = [ @@ -754,7 +768,8 @@ def _add_replicaset_rows( continue if isinstance(obj, ReplicaSetSummary): age = obj.age(self._render_now) - if self._reuse_row(obj, age): + stamp = self._stamp(age) + if self._reuse_row(obj, stamp): continue cells: list[str | Text] = [ obj.name, @@ -766,10 +781,11 @@ def _add_replicaset_rows( ] else: age = obj.age(self._render_now) if isinstance(obj, GenericSummary) else "" - if self._reuse_row(obj, age): + stamp = self._stamp(age) + if self._reuse_row(obj, stamp): continue cells = [obj.name, "", "", "", "", age] - self._emit_row(obj, cells, all_namespaces=all_namespaces, stamp=age) + self._emit_row(obj, cells, all_namespaces=all_namespaces, stamp=stamp) def _add_helm_release_rows( self, rows: list[Summary], *, all_namespaces: bool, pattern: str, presorted: bool = False @@ -781,7 +797,8 @@ def _add_helm_release_rows( if pattern and pattern.lower() not in rel.name.lower(): continue age = rel.age(self._render_now) - if self._reuse_row(rel, age): + stamp = self._stamp(age) + if self._reuse_row(rel, stamp): continue cells: list[str | Text] = [ rel.name, @@ -791,7 +808,7 @@ def _add_helm_release_rows( rel.app_version, age, ] - self._emit_row(rel, cells, all_namespaces=all_namespaces, stamp=age) + self._emit_row(rel, cells, all_namespaces=all_namespaces, stamp=stamp) def _add_helm_revision_rows( self, rows: list[Summary], *, all_namespaces: bool, pattern: str, presorted: bool = False @@ -804,7 +821,8 @@ def _add_helm_revision_rows( if pattern and pattern.lower() not in rev.name.lower(): continue age = rev.age(self._render_now) - if self._reuse_row(rev, age): + stamp = self._stamp(age) + if self._reuse_row(rev, stamp): continue cells: list[str | Text] = [ rev.name, @@ -815,7 +833,7 @@ def _add_helm_revision_rows( rev.description, age, ] - self._emit_row(rev, cells, all_namespaces=all_namespaces, stamp=age) + self._emit_row(rev, cells, all_namespaces=all_namespaces, stamp=stamp) def _add_fallback_rows( self, @@ -835,10 +853,11 @@ def _add_fallback_rows( if pattern and pattern.lower() not in obj.name.lower(): continue age = obj.age(self._render_now) if isinstance(obj, GenericSummary) else "" - if self._reuse_row(obj, age): + stamp = self._stamp(age) + if self._reuse_row(obj, stamp): continue cells: list[str | Text] = [obj.name, *[""] * (width - 2), age] - self._emit_row(obj, cells, all_namespaces=all_namespaces, stamp=age) + self._emit_row(obj, cells, all_namespaces=all_namespaces, stamp=stamp) def _add_package_rows( self, rows: list[Summary], *, all_namespaces: bool, pattern: str, presorted: bool = False @@ -850,7 +869,8 @@ def _add_package_rows( if pattern and pattern.lower() not in pkg.name.lower(): continue age = pkg.age(self._render_now) - if self._reuse_row(pkg, age): + stamp = self._stamp(age) + if self._reuse_row(pkg, stamp): continue cells: list[str | Text] = [ pkg.name, @@ -860,7 +880,7 @@ def _add_package_rows( pkg.description or "-", age, ] - self._emit_row(pkg, cells, all_namespaces=all_namespaces, stamp=age) + self._emit_row(pkg, cells, all_namespaces=all_namespaces, stamp=stamp) fallbacks = [r for r in rows if not isinstance(r, PackageManifestSummary)] self._add_fallback_rows( fallbacks, @@ -880,7 +900,8 @@ def _add_subscription_rows( if pattern and pattern.lower() not in sub.name.lower(): continue age = sub.age(self._render_now) - if self._reuse_row(sub, age): + stamp = self._stamp(age) + if self._reuse_row(sub, stamp): continue cells: list[str | Text] = [ sub.name, @@ -890,7 +911,7 @@ def _add_subscription_rows( sub.state or "-", age, ] - self._emit_row(sub, cells, all_namespaces=all_namespaces, stamp=age) + self._emit_row(sub, cells, all_namespaces=all_namespaces, stamp=stamp) fallbacks = [r for r in rows if not isinstance(r, OLMSubscriptionSummary)] self._add_fallback_rows( fallbacks, @@ -910,7 +931,8 @@ def _add_csv_rows( if pattern and pattern.lower() not in csv.name.lower(): continue age = csv.age(self._render_now) - if self._reuse_row(csv, age): + stamp = self._stamp(age) + if self._reuse_row(csv, stamp): continue cells: list[str | Text] = [ csv.name, @@ -919,7 +941,7 @@ def _add_csv_rows( _csv_phase_cell(csv.phase), age, ] - self._emit_row(csv, cells, all_namespaces=all_namespaces, stamp=age) + self._emit_row(csv, cells, all_namespaces=all_namespaces, stamp=stamp) fallbacks = [r for r in rows if not isinstance(r, CSVSummary)] self._add_fallback_rows( fallbacks, @@ -939,7 +961,8 @@ def _add_generic_rows( if pattern and pattern.lower() not in obj.name.lower(): continue age = obj.age(self._render_now) - if self._reuse_row(obj, age): + stamp = self._stamp(age) + if self._reuse_row(obj, stamp): continue cells: list[str | Text] = [obj.name, age] - self._emit_row(obj, cells, all_namespaces=all_namespaces, stamp=age) + self._emit_row(obj, cells, all_namespaces=all_namespaces, stamp=stamp) diff --git a/tests/ui/test_table_row_memo.py b/tests/ui/test_table_row_memo.py index 231e98ae..66946a16 100644 --- a/tests/ui/test_table_row_memo.py +++ b/tests/ui/test_table_row_memo.py @@ -9,9 +9,12 @@ from __future__ import annotations +from dataclasses import replace from typing import Any +from korvid.core.config import KorvidConfig, ViewConfig from korvid.core.store import Summary +from korvid.k8s.columns import CustomColumn from korvid.k8s.metrics import PodMetrics from korvid.ui.widgets.resource_table import ResourceTable @@ -157,3 +160,32 @@ def counting_format_age(value: str, now: Any = None) -> str: assert len(seen) >= 3, seen assert all(now is not None for now in seen), seen assert len(set(seen)) == 1, seen + + +async def test_replace_view_rows_ignore_hidden_volatile_cells() -> None: + """A `replace: true` custom view keeps only NAME plus the configured + values — all carried by the frozen summary. Nothing volatile survives into + the row, so a metrics poll must not rebuild cells the view discards.""" + samples: dict[tuple[str, str], PodMetrics] = {} + + def lookup(namespace: str, name: str) -> PodMetrics | None: + return samples.get((namespace, name)) + + config = KorvidConfig( + namespace="default", + views={"pods": ViewConfig(columns=(CustomColumn("TEAM", "label", "team"),), replace=True)}, + ) + app = make_app([replace(_pod("alpha"), custom=("payments",))], config=config) + async with app.run_test() as pilot: + table = app.query_one(ResourceTable) + await until(pilot, lambda: table.row_count == 1, label="pods loaded") + view = config.views["pods"] + rows = app.store.get("pods", "default") + table.show("pods", rows, all_namespaces=False, pattern="", metrics=lookup, view=view) + built = _spy_emit(table) + samples[("default", "alpha")] = PodMetrics( + name="alpha", namespace="default", cpu_cores=0.5, memory_bytes=1024 * 1024 + ) + table.show("pods", rows, all_namespaces=False, pattern="", metrics=lookup, view=view) + assert built == [] + assert [str(cell) for cell in table.get_row("default/alpha")] == ["alpha", "payments"] From b340c0f95c72845e104f11e315b5e2eeb8616f99 Mon Sep 17 00:00:00 2001 From: hellices Date: Thu, 6 Aug 2026 22:24:12 +0900 Subject: [PATCH 3/3] test: pin that the memo never freezes AGE for an unchanged summary MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review of #209 pointed out that the clock test uses fresh summary objects, so it bypasses the memo entirely: removing the age string from the stamp would still leave it green while AGE cells froze forever for stable resources. Add `test_age_refreshes_for_an_unchanged_summary_as_time_passes`: it renders the *same* PodSummary object twice with the module clock frozen one minute apart and asserts the AGE cell moves 5m -> 6m. Verified as a real guard by mutation — forcing `_stamp()` to return None fails this test and the metrics test, and passes with the stamp intact. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- tests/ui/test_table_row_memo.py | 42 +++++++++++++++++++++++++++++++++ 1 file changed, 42 insertions(+) diff --git a/tests/ui/test_table_row_memo.py b/tests/ui/test_table_row_memo.py index 66946a16..3a723bbc 100644 --- a/tests/ui/test_table_row_memo.py +++ b/tests/ui/test_table_row_memo.py @@ -10,18 +10,32 @@ from __future__ import annotations from dataclasses import replace +from datetime import UTC, datetime, timedelta from typing import Any +from unittest import mock from korvid.core.config import KorvidConfig, ViewConfig from korvid.core.store import Summary from korvid.k8s.columns import CustomColumn from korvid.k8s.metrics import PodMetrics +from korvid.k8s.models import PodSummary +from korvid.ui.widgets import resource_table from korvid.ui.widgets.resource_table import ResourceTable from .test_app import _pod, make_app from .waits import until +class _FrozenClock: + """Stand-in for `datetime` so a repaint can be aged at a chosen instant.""" + + def __init__(self, instant: datetime) -> None: + self._instant = instant + + def now(self, tz: Any = None) -> datetime: + return self._instant + + def _spy_emit(table: ResourceTable) -> list[str]: """Record the row key of every row whose cells are actually rebuilt.""" built: list[str] = [] @@ -162,6 +176,34 @@ def counting_format_age(value: str, now: Any = None) -> str: assert len(set(seen)) == 1, seen +async def test_age_refreshes_for_an_unchanged_summary_as_time_passes() -> None: + """The memo must not freeze AGE: a pod nobody touches still ages, so the + stamp has to carry the age string and re-render the row when it rolls over.""" + created = (datetime.now(UTC) - timedelta(minutes=5)).isoformat().replace("+00:00", "Z") + pod = PodSummary( + name="alpha", + namespace="default", + phase="Running", + ready="1/1", + restarts=0, + node=None, + created=created, + ) + app = make_app([pod]) + async with app.run_test() as pilot: + table = app.query_one(ResourceTable) + await until(pilot, lambda: table.row_count == 1, label="pod loaded") + rows: list[Summary] = [pod] + table.show("pods", rows, all_namespaces=False, pattern="") + assert str(table.get_row("default/alpha")[-2]) == "5m" + # Same summary object, clock advanced past the next minute boundary. + with mock.patch.object( + resource_table, "datetime", _FrozenClock(datetime.now(UTC) + timedelta(minutes=1)) + ): + table.show("pods", rows, all_namespaces=False, pattern="") + assert str(table.get_row("default/alpha")[-2]) == "6m" + + async def test_replace_view_rows_ignore_hidden_volatile_cells() -> None: """A `replace: true` custom view keeps only NAME plus the configured values — all carried by the frozen summary. Nothing volatile survives into