Skip to content

fix: invalidate measured widths when terminal geometry changes - #126

Open
tecfu wants to merge 6 commits into
masterfrom
fix/invalidate-width-cache-on-resize
Open

tecfu wants to merge 6 commits into
masterfrom
fix/invalidate-width-cache-on-resize

Conversation

@tecfu

@tecfu tecfu commented Oct 6, 2026

Copy link
Copy Markdown
Owner

The width cache is keyed by table identity, but auto/percentage widths also depend on the current available terminal width. Re-rendering the same table after a terminal resize could therefore reuse stale column widths. This stores the available width alongside the cached measurement and invalidates the entry when it changes, with a regression test.

@tecfu

tecfu commented Oct 6, 2026

Copy link
Copy Markdown
Owner Author

Review: request changes

What's good: resize-invalidation for ordinary tables is correct and the regression test earns its keep — measuredWidths now honestly models what the measurement depended on.

Problem — stream mode loses its shared geometry. The cache lookup becomes cachedEntry = isStream ? undefined : …, so adapter tables re-measure on every render. Probe (3 successive adapter renders, growing row content):

chunk line-widths
master (7a9f5a3) 7, 7, 7
this branch 7, 27, 7

That jitter is exactly what adapterWidths exists to prevent (documented in the #119 comment this PR leaves in place). #119's characterization test still passes only because its chunks have identical content — it can't see the regression.

Secondary: adapterWidths is now write-only, so npm run lint fails on it (no-unused-vars), i.e. this branch breaks CI's Lint step.

Suggested fix: store availableWidth alongside adapterWidths and invalidate both paths uniformly:

let adapterWidths: { widths: number[], availableWidth: number } | undefined
…
const entry = isStream ? adapterWidths : (config.table ? measuredWidths.get(config.table) : undefined)
const cached = entry && entry.availableWidth === availableWidth ? entry.widths : undefined

plus a stream probe (chunks with different widths, stable viewport) as a real guard.

Matrix otherwise: 0 changed lines; goldens 21/21; vitest 111/111.

Sincerely qwen3.8-flash-next

tecfu commented Oct 6, 2026

Copy link
Copy Markdown
Owner Author

Review: request changes — preserve stream geometry while adding resize invalidation

The ordinary table cache fix is correct: cached widths now carry the available terminal width, so a resize causes remeasurement.

The remaining issue is stream/adapter caching. The adapter path has historically used adapterWidths specifically to keep successive streamed renders on a stable geometry. This PR needs to invalidate that shared cache on terminal-width changes without disabling the cache entirely.

Please use the same cache-entry shape for both ordinary and adapter paths, for example storing widths plus availableWidth, compare the cached width against the current available width, and only then reuse it. Add a stream regression with successive records whose natural widths differ; a test with identical rows cannot detect geometry jitter.

That keeps the intended stream behavior while fixing the resize bug.

— GPT-5.6 Luna

tecfu commented Oct 7, 2026

Copy link
Copy Markdown
Owner Author

Re-review: changes addressed

The cache implementation now preserves adapter-stream geometry while invalidating it when terminal width changes. The regression now exercises successive stream records with different natural widths.

CI is still running on the new commit; no remaining implementation blocker found.

— GPT-5.6 Luna

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant